Re-consider default for FURB120 (don't pass an argument if it's the same as the default value)

[jab]

This can be bad advice for some APIs. For example:

my_ordered_dict.move_to_end(key1, last=True)
my_ordered_dict.popitem(key2, last=True)

Since these two OrderedDict APIs are notoriously hard for users to remember what the default value of last is, it's preferable to be explicit and always pass last=True, especially for codebases that will be taken over by new, less familiar maintainers over time. This is just one example off the top of my head, but I'm fairly confident that FURB120 is bad advice about as often as it's good advice.

Would you consider flipping FURB120 to be off by default, but enableable via config?

[dosisod]

I agree that certain default values are not very common, and as such, aren't very well known/remembered. In that regard, explicitly passing a default is preferable. This check is somewhat of a double-edged sword, making things cleaner in one instance, but less readable in others. It is hard for Refurb to know when it should or should not emit an error, but I think we can find a way to make things better

When it comes to the standard library functions, they make heavy use of generics, and usually the default values cannot be parsed. As such, the default values (usually pulled from the Python documentation) are hard coded:

https://github.com/dosisod/refurb/blob/9fac0e96c86c013c9b37ff5bd5285ebbb88eef2a/refurb/checks/function/use_implicit_default.py#L61-L68

So until more stdlib functions are added, they shouldn't come up as errors. This doesn't exactly fix the problem, just thought it was worth the mention.

Here are some ideas I have floating around:

• Add a way to "split up" an individual error so that it can be enabled/disabled in certain scenarios. I could imagine disabling this check entirely for stdlib might be preferable, so adding a way to specify --ignore FURB123:stdlib to disable stdlib related errors could solve this.
• Disable this check by default. I am planning on adding this functionality anyways, since I plan to add certain checks which I know will be opt-in. This check, as well as FURB101 (the "read_text()" check) have been pretty controversial, so I might decide to disable it by default.

I would rather find a way to make an error more helpful/user friendly then disabling it by default. Users can globally disable errors pretty easily, so I would only consider disabling errors by default if they are deemed too unwieldy.

Also, does Refurb give an error for the lines you mentioned above? It doesn't on my machine.

[henryiii]

There are cases were it's more readable to include the default, and safer, in case the default changes. For a standard library example, encoding="native" will change to encoding="utf_8" in 3.15. :)

I don't think I've had this check be helpful yet in about a dozen repos.

[jab]

Oh, Refurb doesn't currently give an error for my OrderedDict example, sorry I just assumed it would. Now I'm wondering whether this also counts against enabling this rule by default, since it isn't consistently applied.

+1 to the use case of "lock in the value I need, since the default value may (or definitely will!) be changing". Thanks @henryiii, I actually meant to mention that too when I opened this. For better or worse, defaults change all too often. (This is why Click encourages boolean flags to always have a complement, and I've seen this design come in handy several times.)

The --ignore FURB123:stdlib approach is an interesting idea, though it would not actually help with the use case that prompted me to open this (which occurred outside the standard library). So I'm still hoping this rule will be disabled by default in a future version. Shellcheck provides some good prior art here, where a number of useful checks are disabled by default, since they're not universal enough to enable by default, but are easy to enable when appropriate.

[jab]

For a standard library example, encoding="native" will change to encoding="utf_8" in 3.15. :)

Here's a reference for this, for anyone else who might want to read more about it: https://peps.python.org/pep-0686/

Relatedly, it would be interesting to see other cases where a default value in the stdlib has changed (or is scheduled to change). (I suspect it's happened before and will happen again.)

[dosisod]

Ok, after some thought I agree that this check should be disabled by default. I will go ahead and disable it sometime before the next release.

[jab]

Thanks for reconsidering this!