flake8-bugbear icon indicating copy to clipboard operation
flake8-bugbear copied to clipboard

Warn on unused function arguments

Open taion opened this issue 7 years ago • 10 comments

Ref https://github.com/PyCQA/pyflakes/issues/147

In many cases, unused function arguments are bugs. As such detection behavior would be inappropriate in Pyflakes per the above, the best place to implement such a rule would be here, analogous to the existing B007.

taion avatar Nov 07 '17 19:11 taion

I like the idea in general. The comments on the original pyflakes issue are fair though: if you are implementing a new function that needs to conform to some known signature, ignoring arguments that you don't care about is fine. This includes subclassing, callbacks, etc.

So, Bugbear is going to raise what is arguably a false positive here. How do you suggest us deal with it?

ambv avatar Nov 07 '17 20:11 ambv

For positional args, the same approach as in B007 should work – just prefix the argument with an underscore. Also, pragmatically, we should probably ignore *args.

For keyword args, it's a bit trickier. I would make unused keyword detection a separate rule, and perhaps not enable it by default. This rule should:

  • Ignore **kwargs
  • Ignore abstract methods on classes (not sure how easy this is to detect – in principle some combination of looking for @abstractmethod and raise NotImplementedError() or pass as the function body perhaps)

Personally I'm relatively happy with just # noqa-ing false positives like that, but the unused position arg detection should be fairly nice.

taion avatar Nov 07 '17 20:11 taion

Having this warning would be nice for pytest fixtures too. Some might be expensive to compute and having a warning when they're not used would be very useful.

For context, fixtures in pytest are defined as function arguments.

charlax avatar May 27 '19 12:05 charlax

This would be really useful for me too. Is this issue still being worked on?

ehknight avatar Jul 24 '19 01:07 ehknight

For those interested in this, I've put flake8-unused-arguments up on pypi.

The repo is at https://github.com/nhoad/flake8-unused-arguments/, which includes the documentation on features. I can't edit the description on pypi to include this info until the next release, best I can tell.

nhoad avatar Aug 11 '19 23:08 nhoad

Nice. This makes sense as a separate plugin imo.

Yeah, that's how PyPI works. Just spin a real small release/version increment and join your markdown into the long description. Remember to set that it's Markdown in setup.py like: https://github.com/facebookincubator/ptr/blob/master/setup.py#L53

cooperlees avatar Aug 12 '19 04:08 cooperlees

Yeah I was sure you used to be able to edit descriptions on PyPI, obviously I'm remembering incorrectly though haha. All good though, I did another version bump to get it updated - https://pypi.org/project/flake8-unused-arguments/

nhoad avatar Aug 12 '19 04:08 nhoad

From the issue at https://github.com/PyCQA/pyflakes/issues/147

and there's no best practice which would explicitly declare a parameter as intentionally unused.

Isn't it best practice to prefix unused parameters with _ to mark them as intentionally unused? As in

def foo(_param):
    pass

rnestler avatar Jul 22 '22 15:07 rnestler

Correct, but there are situations where that isn't feasible, e.g. the name might be guarded by someone else's API. You might have something like:

def do_thing(callback):
    # ... do some work...
    callback(value=5)

user = Mock()  # not relevant what it is
def my_callback(user, value):
    user.do_something()

do_thing(functools.partial(my_callback, user))

In my contrived and ugly example, my_callback doesn't use value, but can't rename the argument, because do_thing calls the callback with it as a keyword argument. Things get even more difficult when you don't control do_thing, because you can't change it to use a positional name (thus making the name of the argument in do_thing irrelevant).

nhoad avatar Jul 22 '22 16:07 nhoad

Yeah, I think it's more the recursive nature or callers expecting names of things make this check to hard to implement in my opinion. But down for someone to try and make a calculated test that definitely knows something isn't being used anymore in the function or by callers.

Maybe it could at least check def _methods as the should be private (once again just a practice, not enforced) to a class or module and if nothing else in the file calls with an optional param we could warn there safely maybe. But many other scenarios I feel will be noise.

cooperlees avatar Jul 22 '22 17:07 cooperlees