Split A002 to differentiate between arguments and class-level variables

[clokep]

E.g. with the latest version (1.2.1) I have a few examples of things like this:

class MyClass(object):
    def filter(self, a, b):
        pass

This now fails with a message:

A002 "filter" is used as an argument and thus shadows a python builtin, consider renaming the argument

This text doesn't really make sense (filter isn't an argument).

[dhood]

We are seeing these failures as well with 1.2.1 (as well as some new appropriate failures, thank you!).

+1 for this suggestion, when someone gets the time to address it.

As an additional data point on the message output, ours reference the decorator as opposed to the function:

class GetMap_Response(metaclass=Metaclass):
 ...

    @property
    def map(self):
        """Message field 'map'."""
        return self._map

    @map.setter
    def map(self, value):
        from nav_msgs.msg import OccupancyGrid
        assert \
            isinstance(value, OccupancyGrid), \
            "The 'map' field must be a sub message of type 'OccupancyGrid'"
        self._map = value

gives the following error:

C:/J/workspace/nightly_win_deb/ws/build/nav_msgs/rosidl_generator_py/nav_msgs\srv\_get_map__response.py:67:5: A002 "map" is used as an argument and thus shadows a python builtin, consider renaming the argument
    @property
    ^

C:/J/workspace/nightly_win_deb/ws/build/nav_msgs/rosidl_generator_py/nav_msgs\srv\_get_map__response.py:72:5: A002 "map" is used as an argument and thus shadows a python builtin, consider renaming the argument
    @map.setter
    ^

[gforcada]

@clokep hi! Thanks for using and reporting issues on flake8-builtins!

You are totally right, the error code is wrong, I already fixed it on master. A new release is coming soon.

[gforcada]

@dhood again as @clokep thanks for using and reporting issues!!

Although your issue is physically close/related to @clokep issue, next time, please open a separate bug report 😉

Unfortunately, as far as my ast knowledge goes, it is impossible to get the correct line number and column offset when function definitions have decorators.

I reported it upstream on python issue tracker: see https://bugs.python.org/issue33211

What we can do meanwhile would be to create a new error message (say A003) explaining the situation, only if the function definition has a decorator. Would that be an optimal solution? I would rather not like to parse the file manually and search for the correct spot going line by line just for this corner case... I'm open to pull requests that do that though 😉

[gforcada]

@clokep flake8-builtins 1.2.2 is out, let me know if it is working well for you!

[clokep]

Thanks for replying so quickly! I'm now seeing A001 for those cases ("A001 "filter" is a python builtin and is being shadowed, consider renaming the variable"), which makes more sense.

[dhood]

oh! I misinterpreted the initial post in this issue. I thought the issue being reported was that these failures were occurring at all (it seemed to strict to me). That's why I +1d it, and to me the error message was just a minor detail.

I can see now that this ticket is just about the error message, not about the cause of the failure. Thank you for splitting it into #24

[dirk-thomas]

I just read about this issue and saw that 8b73000 changed the behavior to result in an A001. For a global function it clearly should result in an A001.

I would argue that for the above case though (a method in a classed named after a builtin) there shouldn't be a warning in the first place. How is MyClass.filter shadowing the builtin function filter? Both exist in parallel and don't collide. The method is always identified through either self, an instance variable or the class name. The builtin function is always identified globally. So I don't see why there should be a warning about this in the first place?

[gforcada]

@dirk-thomas you are totally right, although I would say that even if they will be always called by self is usually a bad idea to use those names anyway.

Would it be ok with you to use a separate code for it? Say A003?

[dirk-thomas]

It might be "bad idea" but I would argue that the essence of the warning "thus shadows a python builtin" is not the case. Therefore I don't think there should be a warning about it (or at least the warning shouldn't be enabled by default).

[clokep]

@dirk-thomas That is what I originally meant when I filed this. Note that it does shadow things since classes are their own scope:

class Foobar(object):
    def filter(a, b):
        pass

    # Anywhere in here has the built-in filter shadowed by the one above.

I think that this case is much less likely to cause real issues though, it might make sense to have it off by default?

[dirk-thomas]

Very good point. Some code could do the following which would be shadowing the builtin:

other = filter

A new code sounds good to me then 👍

[dirk-thomas]

Ref for future readers: #28.