Warning to suggest keyword argument when using non-meaningfull litterals as positional argument

[pylint-bot]

Originally reported by: Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore)

---

I see the following idiom a lot in some codebases:

s.splitlines(True)
shutil.rmtree("something", True)
my_particular_function(obj, False)

The following is clearer and improves the code intent:

s.splitlines(keepends=True)
shutil.rmtreee(something, ignore_errors=True)
my_particular_function(obj, remove=False)

We should emit a warning when detecting constants in function calls. Probably True, False, None should be enough, in order to avoid too many false positives. Also, we should ignore builtins. filter(None, something) is okay.

---

• Bitbucket: https://bitbucket.org/logilab/pylint/issue/385

[Pierre-Sassoulas]

For the following call:

a = textwrap.TextWrapper(70, "", "", True, True, False, True, True, False, 4)

It's very hard to understand what the code is doing. Instead you'd want to have keyword arguments:

a = textwrap.TextWrapper(
    width=70,
    initial_indent="",
    subsequent_indent="",
    expand_tabs=True,
    replace_whitespace=True,
    fix_sentence_endings=False,
    break_long_words=True,
    drop_whitespace=True,
    break_on_hyphens=False,
    tabsize=4
)           

On the contrary for :

a.wrap("my string with some content")

Doing the following do not bring much value:

a.wrap(text="my string with some content")

I think a new checker should raise use-keyword-arguments-for-literals (or something better) when we encounter a meaningless literal (not for a string containing characters).

[clavedeluna]

I can work on this. Should this be an extension?

Also to reiterate, this msg should be raised for when arg is used in case of None Bool empty str numbers?? all numbers or just 0? any other cases?

[Pierre-Sassoulas]

I would say we should raise for all numbers. I'm counting on the primers to help us discover the corner case, there's probably a lot of them especially on builtin function used a lot where specifying the kwargs will be just noise.

[clavedeluna]

Sounds good. extension or default checker?

[Pierre-Sassoulas]

Probably an extension, it's safer that way. I think this check is great and will make a lot of code better but as long as we don't have templated configuration it's better to make new checks opt-in.

[altendky]

Thanks for the interest in working on this. As a note, I would be interested in just being able to mandate 'all' parameters that can be passed by name are. In my mind, interesting argument names doesn't mean they are being passed to the proper parameter. I would think that this might be an easy option to implement relative to the rest of the work? For example, if the basic logic is implemented as I am interested in with the differentiation of non-meaningful literals from other arguments being an optional ignore mechanism. Then other ignore mechanisms could be provided as well. Not that I know the workings of writing a pylint extension...

[Pierre-Sassoulas]

Not that I know the workings of writing a pylint extension...

It's surprisingly easy, there's a visitor pattern you need to create a function for the node you want to check. Here's for example a check preventing while loop: https://github.com/PyCQA/pylint/blob/main/pylint/extensions/while_used.py#L32 (https://pylint.pycqa.org/en/latest/development_guide/how_tos/custom_checkers.html)

[clavedeluna]

Can we make a decision on what the first implementation should be:

1. rase on all numbers, bools, None, empty str
2. raise on all parameters that are not passed as a kwarg

I personally vote for 1️⃣ initially

[altendky]

1. sounds like strictly more work. But, you are the one that volunteered time for this. I mostly brought it up to encourage a flexible form where it can later be adapted to support various other needs without the core changing. All unclear values, any positional, more than n positional, etc. But... even if hard coded to 1., it's probably not that hard to adjust the code later anyways so... make what you want and have fun doing it. :]

[Pierre-Sassoulas]

Let's do the easiest option first, then reevaluate depending on the primer's result.

[clavedeluna]

@altendky @Pierre-Sassoulas if you want to take a look at the primer results in https://github.com/PyCQA/pylint/pull/7852 and see what you think. At first glance it definitely feels like a ton of messages. For example, this initial implementation is also flagging not calling default params directly, which does follow the spirit of what we're talking about I think.

Do we want to continue as is or refine it more?

[altendky]

Thanks for all the work here.

I hadn't thought about indicating for parameters with defaults that aren't specified. I see how an implementation could happen to catch these all in one go and that in some sense readability would be enhanced. But, it also suggests that parameter defaults are often bad to leverage and that seems like something to address at the definition, not the call site. I had imagined this a bit more from the other direction, too many positional arguments instead of too few keyword arguments. Anyways, I may be targeting a different case, but even if so it's useful to be able to take a look at your code here.

Just so you can see, if interested, these are some of the things I would like to have CI block from getting merged.

• https://github.com/Chia-Network/chia-blockchain/blob/898543b87433e9bd7c9495b4f9b6e2d6499de144/chia/consensus/block_creation.py#L489-L517
• https://github.com/Chia-Network/chia-blockchain/blob/898543b87433e9bd7c9495b4f9b6e2d6499de144/chia/consensus/full_block_to_block_record.py#L147-L173
• https://github.com/Chia-Network/chia-blockchain/blob/898543b87433e9bd7c9495b4f9b6e2d6499de144/chia/seeder/crawl_store.py#L291-L312
• https://github.com/Chia-Network/chia-blockchain/blob/898543b87433e9bd7c9495b4f9b6e2d6499de144/chia/simulator/block_tools.py#L700-L730

Aside from such extreme cases, I do also tend to prefer passing by name even for just a single parameter or two. I'm sure I'm at an extreme on that point though.

Thanks again.

[clavedeluna]

@altendky your linked examples definitely point in the direction of issuing the message when there are 2+ arguments which is what is suggested here https://github.com/PyCQA/pylint/pull/7852#issuecomment-1328046181

However I'm having a pretty hard time excluding kwargs with defaults. @Pierre-Sassoulas maybe we can do 2+ arguments of all types for now and improve later on?

[Pierre-Sassoulas]

I think we need to be able to distinguish what is an implicit default arguments and what is explicitly defined by the user or the check will be too noisy to be usable (especially for function with a lot of optional args). I don't have the bandwidth to check how it can be done but the code for Call and possibly CallContext in astroid should help.