search

ros-infrastructure / catkin_pkg

Standalone Python library for the catkin build system.
https://github.com/ros/catkin
Other
47 stars 91 forks source link

Fix flake8-import-order issues #219

Closed Nickolaim closed 6 years ago

Nickolaim commented 6 years ago

I100 Import statements are in the wrong order. 'import os' should be before 'import docutils.core' and in a different group. I101 Imported names are in the wrong order. Should be PackageTemplate, create_package_files I201 Missing newline between import groups. 'import argparse' is identified as Stdlib and 'from future import print_function' is identified as Future.

mikaelarguedas commented 6 years ago

change looks good to me. I don't know what strategy maintainers of this package prefer to adopt. In ROS 2.0 we follow a case insensitive import order, strategy also named googlestyle in flake8. Using that style here would make the change of this PR minimal for files like this one where the import order is already complying with the googlestyle import order.

Nickolaim commented 6 years ago

Sounds good, I will update it as well. Note for self: import-order-style = google

Nickolaim commented 6 years ago

Apparently changing default import-order-styleis problematic. Setting import_order_style='google' while calling get_style_guide() has no effect - when plugin is initialized, it uses only settings from command line and configuration file - details are in flake8/legacy.py.

ROS2 has a solution for this problem - it passes options to parse_configuration_and_cli(), see ament_flake8/main.py.

In general, it does not mean that this is not possible to solve - moving configuration to a file is one way. But this definitely out of the scope of this change.

mikaelarguedas commented 6 years ago

Sounds good to me, thanks for giving it a shot :+1:

Nickolaim commented 6 years ago

Any suggestions for this PR?

dirk-thomas commented 6 years ago

When enabling flake8-import-style the same style should be preserved which the code currently already adheres to (at least mostly if not everywhere) which is the google style. This can e.g. be configured with a .flake8 file in the root of the package (e.g. https://github.com/ament/ament_package/blob/a6c73aeff77c188597211fee9f9e34508487b531/.flake8).

Nickolaim commented 6 years ago

PR #222 moves flake8 configuration from the source code into .flake8 configuration file. When it is merged, I update the settings to follow google import style.

Nickolaim commented 6 years ago

Google import order style has been applied.

Nickolaim commented 6 years ago

Both comments from the review are resolved.

dirk-thomas commented 6 years ago

Thanks!