Test python module import order using flake8 #63

dhood · 2016-10-18T06:55:00Z

connects to #53
requires ros2/ci#3

This currently adds flake8 as a linter which has pep8 and pyflakes checks included by default. I have added the flake8-import-order plugin for flake8 in ros2/ci#3.

This can replace the ament_pep8 and ament_pyflakes linter packages if appropriate, and with the pep257 plugin it can replace the ament_pep257 package as well. Should I go ahead and do that?

These are the files that are failing the new linter test: http://ci.ros2.org/job/ci_linux/1783/testReport/ using the Google style guide (some will be fixed by PyCQA/flake8-import-order#94)

dirk-thomas · 2016-10-18T15:49:43Z

ament_cmake_flake8/CMakeLists.txt

@@ -0,0 +1,23 @@
+cmake_minimum_required(VERSION 2.8.3)


We require version 3.5.

dirk-thomas · 2016-10-18T15:50:30Z

ament_cmake_flake8/ament_cmake_flake8-extras.cmake

@@ -0,0 +1,22 @@
+# Copyright 2014-2015 Open Source Robotics Foundation, Inc.


The copyright year should be 2016.

Same below

dirk-thomas · 2016-10-18T15:57:43Z

ament_cmake_flake8/package.xml

+    The CMake API for ament_flake8 to check code syntax and style conventions
+    with flake8.
+  </description>
+  <maintainer email="dthomas@osrfoundation.org">Dirk Thomas</maintainer>


This should be your name.

Same below.

dhood · 2016-10-18T18:04:15Z

comments addressed in ee080bc. replace pep8, pyflakes, and pep257 packages?

dirk-thomas · 2016-10-18T18:17:19Z

ament_lint_common/package.xml

@@ -16,6 +16,7 @@
  <exec_depend>ament_cmake_cppcheck</exec_depend>
  <exec_depend>ament_cmake_cpplint</exec_depend>
  <exec_depend>ament_cmake_lint_cmake</exec_depend>
+  <exec_depend>ament_cmake_flake8</exec_depend>


Please move this line one up to be in alphabetical order.

dirk-thomas · 2016-10-18T18:19:23Z

Has anyone successfully setup this linter in the editor? I have pycodestyle, pydocstyle, and pyflakes working but not yet flake8.

wjwwood · 2016-10-18T18:20:40Z

I only use flake8 in Sublime.

wjwwood · 2016-10-18T18:23:01Z

Sorry, hit send on accident. I use this plugin:

https://github.com/SublimeLinter/SublimeLinter-flake8

I think there is an atom one too:

https://atom.io/packages/linter-flake8

I haven't tried vim or emacs.

dirk-thomas · 2016-10-18T18:48:11Z

I had to configure the Atom package linter-flake8 to use the flake8 binary installed via pip (since the Ubuntu package is too old to support on-the-fly linting (while you type, no need to save the file, passing the content via stdin).

dirk-thomas · 2016-10-18T20:57:40Z

ament_flake8/ament_flake8/main.py

+        return super(CustomStyleGuide, self).input_file(filename, **kwargs)
+
+
+class CustomReport(pep8.StandardReport):


Is there a way to implement this without using pep8 directly (which the package doesn't declare a dependency on)?

dhood#1 gets rid of the pep8 usage.

dhood · 2016-10-19T18:58:01Z

following from ros2/ci#3 (comment): this uses flake8 v2.x. If not using flake8 3.x instead of 2.x is a blocker, I'll put this on the backburner since it'll likely take me a bit of effort to get it to work with 3.x (and this isn't particularly high priority)

FYI I saw your issue/PR on flake8-import-order re the import order capitalisation sensitivity of the google style guide: I had already opened PyCQA/flake8-import-order#94 to address it (it was referenced in this PR but I could have been clearer)

…led by flake8 anymore

The current implementation of get_style_guide does not process the config file correctly.

dhood · 2017-01-04T04:31:58Z

OK, this should be good for review again please

dirk-thomas · 2017-01-04T05:34:41Z

What is the fallback behavior if only version 2 of flake8 is available since that will be the case when we create Debian packages on Xenial at some point?

dirk-thomas · 2017-01-04T05:59:33Z

Are we interested in any of the other flake8 plugins in this list?

Several of these plugins look interesting. I would suggest to get the current minimum set merged before considering to add more though.

dhood · 2017-01-04T06:06:51Z

with only flake8 v2.x the test will be added, but will fail (implemented in 81e6f10)

I felt it would be misleading if the test still passes.

Would it make sense to add a version check in the hook to stop it from getting added in that case? https://github.com/ament/ament_lint/pull/63/files#diff-da8d8d587f53d8cedf3b2cd20757fc40R16
I thought it might not make sense, since setups might be different when tests are added vs run.

Alternatively, I did have it working with v2.x originally, so we can choose to support both

dhood · 2017-01-04T06:10:45Z

there's also the option of adding a flake8>3.0 requirement to the setup.py of ament_flake8 but since other ament linters don't do this I figured we still want to be able to install the package even without flake8 installed at all

dirk-thomas · 2017-01-04T16:22:40Z

Once flake8 provides pep8 / pep257 linting we will likely remove those from ament_lint_common. At that point I think flake8 should still provide that functionality even if it is "only" run on Xenial with Debian dependencies installed (which implies flake8 2.x). It will only not provide the include-order checks but that is an acceptable fallback in my opinion. Therefore I think the package should gracefully fall back and support older flake8 versions.

dhood · 2017-01-04T23:54:49Z

ok bf07f5b adds support for both.

The tests will fail with v3, however, because the --cover-inclusive flag that gets set for all ament_python tests causes the legacy file to be run, so I'm still working on a fix for that.

otherwise it's not present in xunit files for tests run directly with nose (not ament_cmake_flake8)

dhood · 2017-01-10T11:05:45Z

tests can run if ament/ament_tools#130 is used . That PR removes the cover-inclusive option but it will affect more packages than just this one, so I'm open to other (more local) suggestions for how to avoid the legacy file causing coverage tests to fail. (exceptions and sys.exit make the test fail)

Regarding building debs, here's a job with flake8 installed from apt (i.e. v2) and no import order (using https://github.com/ros2/ci/compare/flake8-apt), which I understood is what we would have for debian packages: (notice fewer linter errors, and pip freeze lists flake8 v2.5.4):

00:04:17.872 ==> "/usr/bin/python3" -m pip freeze
00:04:18.243 coverage==3.7.1
00:04:18.243 empy==3.3.2
**00:04:18.243 flake8==2.5.4**
00:04:18.244 funcsigs==0.4
00:04:18.244 mccabe==0.2.1
00:04:18.244 mock==1.3.0
00:04:18.244 nose==1.3.7
00:04:18.244 pbr==1.8.0
00:04:18.244 pep8==1.7.0
00:04:18.244 pexpect==4.2.1
00:04:18.244 ptyprocess==0.5.1
00:04:18.244 pydocstyle==1.0.0
00:04:18.244 pyflakes==1.1.0
00:04:18.244 PyYAML==3.11
00:04:18.244 six==1.10.0
00:04:18.244 vcstool==0.1.21
00:04:18.244 virtualenv==15.1.0

so I think it should be good for debian packaging.

dhood · 2017-01-10T20:29:38Z

I'll just merge the legacy.py file causing issues with the coverage job into main.py. The file will be a bit messier but it won't require changes to how coverage is run on ament_python packages (still open to ideas for alternatives if anyone has run into this issue before)

dirk-thomas · 2017-01-10T20:33:04Z

Merging those two files will make it more effort to remove the legacy code in the future. Is there really no way to preserve the separation?

dhood · 2017-01-12T02:48:02Z

OK I've fixed it. rather than trying to prevent the file from being imported, I just prefixed the whole file with if LooseVersion(flake8.__version__) < '3.0': (036f3c2#diff-aa7c672db8ce0dbbacbace23d3cbd10eR23). So this PR no longer requires ament/ament_tools#130

these are the normal jobs (using ros2/ci#3, which gets flake8 v3, from pip):
Linux: http://ci.ros2.org/job/ci_linux/2223/#showFailuresLink (25 failed tests)
OS X: http://ci.ros2.org/job/ci_osx/1685/testReport/ (rttest and pendulum_control skipped (-4) + 2 non-lint test failures = 23)
Windows: http://ci.ros2.org/job/ci_windows/2212/testReport/ (rttest and pendulum_control skipped (-4) + 1 non-lint test failures = 22)

this is linux using flake8 from apt (v2) and import-order from pip (same result as above):
http://ci.ros2.org/job/ci_linux/2227

and this is what it would be like if we are using flake8 v2 and no import-order plugin (as would be the case for debians) - no issues if the plugin is not found:
http://ci.ros2.org/job/ci_linux/2224/testReport/

I have matching PRs for the linter fixes that I'll open once this gets approved.

mikaelarguedas · 2017-01-12T04:04:29Z

Any reason to delay the PRs with the linter fixes until this one gets approved?

dhood · 2017-01-12T04:06:59Z

I figured that depending on the outcome of this PR, the changes required to make linters pass for other packages might change

dirk-thomas · 2017-01-12T16:38:26Z

This looks goo to me. With another round of CI which uses your linter fixes this should be good to go.

dhood · 2017-01-31T02:08:48Z

CI jobs with fixes in PRs attached to #53

OS X:
Linux (no linter failures):
Windows (no linter failures):

mikaelarguedas · 2017-01-31T07:22:25Z

Can be done in a follow up but it would be great to run flake8 on ament_python packages as well.

Otherwise all attached fixes look good to me 👍

dirk-thomas · 2017-01-31T16:31:32Z

ament_lint_cmake/package.xml

@@ -11,6 +11,7 @@
  <license>Apache License 2.0</license>

  <test_depend>ament_copyright</test_depend>
+  <test_depend>ament_flake8</test_depend>


Are there any cases which are covered by pep8 / pep257 / pyflakes which are not covered by flake8? If not I would suggest to remove the redundant tests (ament_pep8, ament_pep257, ament_pyflakes).

dirk-thomas · 2017-01-31T16:31:58Z

ament_lint_common/package.xml

@@ -16,6 +16,7 @@
  <exec_depend>ament_cmake_copyright</exec_depend>
  <exec_depend>ament_cmake_cppcheck</exec_depend>
  <exec_depend>ament_cmake_cpplint</exec_depend>
+  <exec_depend>ament_cmake_flake8</exec_depend>


Are there any cases which are covered by pep8 / pep257 / pyflakes which are not covered by flake8? If not I would suggest to remove the redundant tests (ament_pep8, ament_pep257, ament_pyflakes).

It should cover pep8 and pyflakes, yes. I'll remove ament_pep8 and ament_pyflakes from ament_lint_common so that they are not added when ament_lint_auto is used.

I haven't added the docstrings flake8 plugin. Since you mentioned the plugins won't be installable when we build debians, perhaps that's a reason to leave ament_pep257 as is - what do you think?

Sounds good to me.

dirk-thomas · 2017-03-17T19:38:51Z

ament_flake8/ament_flake8/main.py

+        print('%d errors' % (report.total_errors))
+
+        print('')
+        error_type_counts = report.get_error_type_counts()


This line fails when the legacy API is being used since this class doesn't have that method.

thanks for the heads up. I'll fix it

dirk-thomas · 2017-06-21T20:22:14Z

ament_flake8/ament_flake8/main.py

+
+    flake8_argv = []
+    flake8_argv.append('--config={0}'.format(config_file))
+    flake8_argv.append('--exclude={0}'.format(excludes))


This line doesn't work. See #77

* Add flake8 linter * Don't deal with flake8-import-order just yet * Debugging prints * Reinstate import order rule * Fix reporting bug by using the inner flake8 style guide * Fixup * Add comment on wrapper StyleGuide use * use flake8 v3 (#1) * Reorder package.xml * Get the filenames from the file checkers because input_file isn't called by flake8 anymore * Output count of all error types * Get flake8 to use the config file The current implementation of get_style_guide does not process the config file correctly. * Error when flake8 v2 found * Print errors like pep8 * remove __future__ imports * add schema to manifest files * Support flake8 v2 as well as v3 * Output checked files otherwise it's not present in xunit files for tests run directly with nose (not ament_cmake_flake8) * Prevent v2 imports from happening on systems with v3 * Flake8 replaces pep8+pyflakes

dhood added the in review Waiting for review (Kanban column) label Oct 18, 2016

dhood self-assigned this Oct 18, 2016

dirk-thomas reviewed Oct 18, 2016

View reviewed changes

dhood mentioned this pull request Oct 18, 2016

Install flake8 and flake8-import-order ros2/ci#3

Merged

dhood and others added 14 commits January 4, 2017 13:12

Add flake8 linter

c9e1d5c

Don't deal with flake8-import-order just yet

9f8106a

Debugging prints

c45cefc

Reinstate import order rule

37a969d

Fix reporting bug by using the inner flake8 style guide

b900d4f

Fixup

91cd4c7

Add comment on wrapper StyleGuide use

231ff15

use flake8 v3 (#1)

50e650f

Reorder package.xml

495d29e

Get the filenames from the file checkers because input_file isn't cal…

3d0f3c9

…led by flake8 anymore

Output count of all error types

521bf78

Get flake8 to use the config file

256264a

The current implementation of get_style_guide does not process the config file correctly.

Error when flake8 v2 found

81e6f10

Print errors like pep8

2b24b18

dhood force-pushed the flake8 branch from 2b1d2c6 to 2b24b18 Compare January 4, 2017 02:18

dhood added 2 commits January 4, 2017 13:31

remove __future__ imports

c929674

add schema to manifest files

c8fce1b

Support flake8 v2 as well as v3

bf07f5b

Output checked files

3b888fc

otherwise it's not present in xunit files for tests run directly with nose (not ament_cmake_flake8)

Prevent v2 imports from happening on systems with v3

036f3c2

dirk-thomas reviewed Jan 31, 2017

View reviewed changes

Flake8 replaces pep8+pyflakes

be9e083

dirk-thomas approved these changes Jan 31, 2017

View reviewed changes

dhood mentioned this pull request Feb 1, 2017

Replace pep8+pyflakes with flake8 ros2/rosidl#203

Merged

dhood merged commit d079913 into ament:master Feb 1, 2017

dhood removed the in review Waiting for review (Kanban column) label Feb 1, 2017

dhood mentioned this pull request Feb 7, 2017

Don't add cover-inclusive option ament/ament_tools#130

Closed

dirk-thomas reviewed Mar 17, 2017

View reviewed changes

dhood deleted the flake8 branch March 23, 2017 23:25

dirk-thomas reviewed Jun 21, 2017

View reviewed changes

		@@ -0,0 +1,22 @@
		# Copyright 2014-2015 Open Source Robotics Foundation, Inc.

		return super(CustomStyleGuide, self).input_file(filename, **kwargs)


		class CustomReport(pep8.StandardReport):

Test python module import order using flake8 #63

Test python module import order using flake8 #63

Conversation

dhood commented Oct 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Oct 18, 2016 • edited

Choose a reason for hiding this comment

dirk-thomas commented Oct 18, 2016

wjwwood commented Oct 18, 2016

wjwwood commented Oct 18, 2016

dirk-thomas commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Oct 19, 2016

dhood commented Jan 4, 2017

dirk-thomas commented Jan 4, 2017

dirk-thomas commented Jan 4, 2017

dhood commented Jan 4, 2017

dhood commented Jan 4, 2017

dirk-thomas commented Jan 4, 2017

dhood commented Jan 4, 2017

dhood commented Jan 10, 2017

dhood commented Jan 10, 2017

dirk-thomas commented Jan 10, 2017

dhood commented Jan 12, 2017

mikaelarguedas commented Jan 12, 2017

dhood commented Jan 12, 2017

dirk-thomas commented Jan 12, 2017

dhood commented Jan 31, 2017 • edited

mikaelarguedas commented Jan 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Oct 18, 2016 •

edited

dhood commented Oct 18, 2016 •

edited

dhood commented Jan 31, 2017 •

edited

mikaelarguedas commented Jan 31, 2017 •

edited