Flake 8 builtins failures

[dhood]

All nightlies had linter failures from a flake8-builtins update; looking into it

Changelog:
https://github.com/gforcada/flake8-builtins/blob/master/CHANGES.rst#12-2018-04-01

Example failure: https://ci.ros2.org/view/nightly/job/nightly_win_deb/834/testReport/junit/visualization_msgs/flake8_rosidl_generated_py/A002__C__J_workspace_nightly_win_deb_ws_build_visualization_msgs_rosidl_generator_py_visualization_msgs_msg__marker_py_276_5_/

[dhood]

Someone has already opened an issue that the latest change is too strict: gforcada/flake8-builtins#22

I will pin the previous version to not affect CI and fix the new issues that were reported correctly.

[dhood]

pinned in ros2/ci#140 and failure addressed in ros2/ros2cli#85; will leave this open until we un-pin

[dhood]

so, I misinterpreted gforcada/flake8-builtins#22. apparently this is a reasonable linter failure and we need to either change our generated code to comply with the linter, or ignore that linter code on those lines.

This is an example of what causes the linter failure (it comes from generated message/service code):

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
    ^

(the error output is a bit misleading but that's a different conversation).

@ros2/team would you agree that we should NOQA lines that might cause this flake8-builtins failure, e.g. https://github.com/ros2/rosidl/blob/a416f1323fe27059dc310dd5cbed6ff291a3c5c6/rosidl_generator_py/resource/_msg.py.em#L174 ?

[mikaelarguedas]

I guess the general policy is to generate variables with trailing underscore when they collide with language builtins. Would it make sens to do that here?

Edit: may not be trivial or worth doing. I'm not against ignoring the linter on that line, just wanted to point out a potential alternative

[dhood]

I had understood that these functions are the interface that we present to users for accessing/filling messages, which is why I thought continuing to use the existing field names as the function names makes sense (my understanding may be off though)

[mikaelarguedas]

I thought we were renaming field name colliding with builtins in the generators to avoid breakage later on. Apparently we don't, and the generator fails the hard way is you use reserved C or C++ names (I just tried adding a field named class to a message definition and 💥 )

[dirk-thomas]

I had understood that these functions are the interface that we present to users for accessing/filling messages, which is why I thought continuing to use the existing field names as the function names makes sense

Ignoring the decorator / property I don't see why a method name of a class should result in a warning at all. I commented on the upstream ticket to get clarification on that.

[dhood]

the conclusion upstream has been that the error is valid but is now reported as A003.

noqa A003 on lines like this? https://github.com/ros2/rosidl/blob/a416f1323fe27059dc310dd5cbed6ff291a3c5c6/rosidl_generator_py/resource/_msg.py.em#L175 (I'm assuming noqa can be done for a specific code in that context, but haven't tried)

[dirk-thomas]

Yes, noqa works on a def line.

Not sure if we want to blindly add it to the line or only when the function name is in a list of builtin names.

[dhood]

The main reason to only put it if necessary is for readability, is that right? Since those generated files are more "internal" than something users interact with, I think it would be fine?
Or is there another reason to only put it if required?

[dirk-thomas]

The main reason to only put it if necessary is for readability, is that right?

Yes.

Since those generated files are more "internal" than something users interact with, I think it would be fine? Or is there another reason to only put it if required?

Users reading the generated message class might be confused why this is there even though it isn't applicable to that line. It is not necessary to suppress it - it would just be nicer / cleaner.

[dhood]

I agree that it would be nice to suppress it if the cost is low enough.

I don't think it's worth pulling in the build dependency on flake8-builtins to get its exact builtin list/whitelist for this purpose, but it'd be trivial to duplicate its approach (even just roughly), as an in-between

[mikaelarguedas]

@dhood Will this be addressed for bouncy ? Or should we modify the installation instructions to pin the flake8 version?

[dhood]

unpinning of flake8-builtins has been done in ros2/ci#203 (and noqa addition in ros2/rosidl_python#6 and ros2/launch#100). For tracking purposes: it was not done in time for the bouncy release.