Enable flake8 #3224

hmaarrfk · 2018-06-27T04:45:13Z

Flake8 was enabled, but we were ignoring the output so it might as well have been removed.
That said, it is useful to check unused variables, and undefined variables.

I disabled most of the style options that don't make sense when you write mathematical code,
added a few # noqa to keep the useful warnings for future code.

Things are mostly split by logical modifications in a few different commits

I setup flake8 such that it would be much less strict than before and only run on a single build

For reviewers

(Don't remove the checklist below.)

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

codecov-io · 2018-06-27T15:53:23Z

Codecov Report

Merging #3224 into master will decrease coverage by 0.99%.
The diff coverage is 66.66%.

@@           Coverage Diff            @@
##           master    #3224    +/-   ##
========================================
- Coverage   87.82%   86.82%    -1%     
========================================
  Files         325      340    +15     
  Lines       27317    27486   +169     
========================================
- Hits        23992    23866   -126     
- Misses       3325     3620   +295

Impacted Files	Coverage Δ
skimage/viewer/plugins/overlayplugin.py	`91.07% <ø> (ø)`	⬆️
skimage/draw/draw.py	`98.94% <ø> (ø)`	⬆️
skimage/morphology/grey.py	`94.33% <ø> (ø)`	⬆️
skimage/filters/_frangi.py	`97.05% <ø> (ø)`	⬆️
skimage/setup.py	`8.1% <0%> (ø)`
skimage/_shared/utils.py	`86.66% <100%> (ø)`	⬆️
skimage/viewer/qt.py	`88.57% <100%> (ø)`	⬆️
skimage/_shared/version_requirements.py	`83.05% <100%> (ø)`	⬆️
skimage/segmentation/random_walker_segmentation.py	`92.01% <100%> (-1.88%)`	⬇️
skimage/viewer/plugins/lineprofile.py	`96.42% <100%> (ø)`	⬆️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 133aa70...b15243e. Read the comment docs.

hmaarrfk · 2018-09-30T18:56:35Z

@scikit-image/core I don't know if you care about flake8, but I find it gives much better error message than running code through python when you have a syntax error. The commit message are now much improved compared to the first version I submitted a few months ago.

soupault · 2018-10-02T17:25:09Z

.travis.yml

@@ -38,7 +38,7 @@ matrix:
  include:
    - os: linux
      python: 3.5
-      env: OPTIONAL_DEPS=1 WITH_PYSIDE=1 BUILD_DOCS=1
+      env: OPTIONAL_DEPS=1 WITH_PYSIDE=1 BUILD_DOCS=1 TEST_FLAKE8=1


RUN_FLAKE8 or smth?

soupault · 2018-10-02T17:26:10Z

.travis.yml

+    # catching it.
+    # Therefore, it is best to run flake8 first, on a single build
+    # with the oldest version of python we support
+    - if [[ $TEST_FLAKE8 == "1" ]]; then


Should all of this be in a dedicated tools\flake8.py?

done. flake8.sh

soupault · 2018-10-02T17:26:57Z

.travis.yml

+    # with the oldest version of python we support
+    - if [[ $TEST_FLAKE8 == "1" ]]; then
+        pip install flake8;
+        flake8;


You probably need to keep the same flake8 arguments as before.

I think it is all in the setup.cfg. I added a dummy commit to trip up flake8 to test it.

soupault · 2018-10-02T17:27:37Z

skimage/io/_plugins/util.py

-    CPU_COUNT = multiprocessing.cpu_count()
-except:
-    CPU_COUNT = 2
+import multiprocessing


Already in. #3434

Yeah, that was cherry picked from here. Rebasing got rid of it.

hmaarrfk · 2018-10-02T18:57:48Z

@soupault see this build https://travis-ci.org/scikit-image/scikit-image/jobs/436291887#L905 showing we don't need to setup any flake8 command line arguments.

soupault

Looks good to me! Mark, could you rebase?

hmaarrfk · 2019-02-27T19:19:45Z

Yeah. Let me think about this again. Flake8 is a little bit of an annoyance when it causes something to fail for no reason. This is especially true when they change their mind about the style guide and whatnot.

The worst part about this script, is that if somebody else introduces a flake8 error, all future contributors would see red.

My goal is to have contributors get to green as easily as possible. The goal of these tests should be to guide contributors through the mundane stuff (style, and test infrastructure), while helping them with the technical parts.

soupault · 2019-02-27T19:32:17Z

@hmaarrfk well, a build failing with a redundant whitespace is not what I would like to see either. But having flake8 executed during the early phase of CI routine is pretty useful.
You're right, flake8 is PR-agnostic in this case, and may lead to a chain of red builds.

What is we adjust pep8speaks bot to perform only the most significant checks (as, currently, specified in the PR), and remove flake8 completely (or, at least, disable its influence on the build status)?

Other changes in the PR are very welcome.

Borda · 2019-04-26T23:08:36Z

setup.cfg

@@ -1,2 +1,47 @@
 [metadata]
 license_file = LICENSE.txt
+
+[flake8]
+ignore =


you can use full names instead of shortcuts

my bad, it seems that it works only for pylint

Borda · 2019-04-26T23:08:58Z

setup.cfg

+    W391
+
+exclude =
+    test_*,


why excluding tests?

Tests often include many data arrays, that are well justified not to follow pep8.

@hmaarrfk this is true, but test functions themselves should follow PEP8. I'd prefer finer-grained control here. Maybe it's not doable but I prefer to ignore these warnings rather than silence them completely.

I really believe that warnings should be set to 0, except when they would add more noise (like in examples).

The idea is that currently the flake8 stuff is just fully being ignore.

Borda · 2019-04-26T23:09:57Z

skimage/io/__init__.py

-from .manage_plugins import *
+from .manage_plugins import (use_plugin, call_plugin, plugin_info,
+                             plugin_order, reset_plugins,
+                             find_available_plugins, available_plugins)
 from .sift import *


why here is not the names list too?

you could. I think i gave up on changing style, I mostly like flake8 to find python typos, and not imposing style.

Borda · 2019-04-26T23:11:13Z

skimage/measure/mc_meta/createluts.py

@@ -134,10 +134,11 @@ def search_line(lines, refline, start=0):



-def getLutNames(prefix):
+def getLutNames(prefix, luts):


from the review scope, it is not clear but luts does not seems to be used

Borda · 2019-04-26T23:12:13Z

skimage/segmentation/random_walker_segmentation.py

@@ -30,7 +30,7 @@ def new_del(self):
            pass
    umfpack.UmfpackContext.__del__ = new_del
    UmfpackContext = umfpack.UmfpackContext()
-except:
+except:  # noqa


an exception should be specified et leas by Exception

I agree, and here we can probably do even better. @JDWarner might have some thoughts?

These kind of maintenance changes really require 3 devs working according to our policies. Will not work with such large sweeping changes without focused effort ;)

stefanv · 2019-12-06T00:49:31Z

Love this; thanks @hmaarrfk! Needs a quick rebase, hopefully not too honerous (err, may well be).

Reminds me about the conversations around black, @jni. I'd be okay with the way it's used in Napari, e.g.

blank lines non multiple of 4 tabbing trailing white space

getLutNames is still untested or uncalled in the whole codebase though

jni · 2019-12-06T13:31:46Z

Yeah, pre-commit hooks are the right way to implement these things. Unfortunately, though, it makes it harder to get started developing for skimage...

hmaarrfk · 2019-12-07T00:50:26Z

THe flake8 changes I had to actually enact are things like tabulation, and trailing whitespace, I see these as "minor" and better to deal with near the end of the PR, instead of upfront with a pre-commit hook

stefanv · 2019-12-07T00:55:44Z

THe flake8 changes I had to actually enact are things like tabulation, and trailing whitespace, I see these as "minor" and better to deal with near the end of the PR, instead of upfront with a pre-commit hook

Agreed, for now. My suggestion was for the long term, where I think we would benefit from enforced compliance that simultaneously relieves the burden on the contributor.

I don't think it's much of a problem to help new users with this. For other projects, I have a single command they can run to install the commit hook, e.g. make lint-install (or, perhaps, python .tools/install-lint.py.

doc/release/contribs.py

grlee77 · 2020-10-29T18:51:55Z

This now needs a rebase, but I would be fine with merging once the CI is green. We can always refine things again later as needed.

stefanv · 2023-02-06T18:49:34Z

Implemented using ruff in #6729

This comment has been minimized.

Sign in to view

hmaarrfk mentioned this pull request Jun 27, 2018

Fix the human readable error message on a bad build. #3223

Merged

4 tasks

hmaarrfk force-pushed the flake8 branch 6 times, most recently from e0f42e1 to 1d78e82 Compare June 27, 2018 15:53

hmaarrfk force-pushed the flake8 branch 3 times, most recently from 4b0a6c4 to 101c0e7 Compare September 30, 2018 18:54

hmaarrfk force-pushed the flake8 branch from 101c0e7 to 0816459 Compare September 30, 2018 19:45

soupault reviewed Oct 2, 2018

View reviewed changes

soupault added the 🔧 type: Maintenance Refactoring and maintenance of internals label Oct 2, 2018

hmaarrfk force-pushed the flake8 branch 3 times, most recently from e2b3d73 to d0c9522 Compare October 2, 2018 18:48

hmaarrfk force-pushed the flake8 branch 2 times, most recently from 7cb9161 to 3b8fe83 Compare October 6, 2018 19:23

hmaarrfk force-pushed the flake8 branch from 3b8fe83 to b15243e Compare November 3, 2018 15:39

soupault approved these changes Feb 27, 2019

View reviewed changes

Borda reviewed Apr 26, 2019

View reviewed changes

Borda mentioned this pull request Apr 27, 2019

Documentation for color images in histogram matching #3865

Open

5 tasks

flake8: Fix invisible characters

764e677

blank lines non multiple of 4 tabbing trailing white space

hmaarrfk added 13 commits December 6, 2019 00:23

flake8: remove unused variables from benchmarks

90cb115

flake8: touchup mailmap test for membership

890f408

flake8: don't assing a lambda expression in setup.py

841ebca

flake8: touchup tests for identify and membership in helper functions

0ac1567

flake8: each line should only have one statement.

39a26d5

flake8: remove trailing whitespace

f501e99

MNT: a variable luts was missing from getLutNames

87a60af

getLutNames is still untested or uncalled in the whole codebase though

MNT: be explicit about what we import from io.manage_plugins

35d6b6f

Build tools: flake8 is now enabled in travis

3845d16

make flake8 exit 0

4538160

Add flake 8 back to the regular test script.

79b2772

Delete flake8.sh

28a60e6

Update script.sh

f9206ef

hmaarrfk force-pushed the flake8 branch from e8f4a8c to f9206ef Compare December 6, 2019 05:26

hmaarrfk added 2 commits December 6, 2019 19:47

Fixup new flake8 errors

86495e4

Conditionally run flake8 on a single build

f436208

grlee77 reviewed Oct 28, 2020

View reviewed changes

doc/release/contribs.py Outdated Show resolved Hide resolved

Delete contribs.py

161e35d

Base automatically changed from master to main February 18, 2021 18:23

mkcor mentioned this pull request Feb 22, 2021

2021's calendar of community management #5169

Closed

grlee77 added the 🤖 type: Infrastructure CI, packaging, tools and automation label Apr 5, 2021

stefanv closed this Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable flake8 #3224

Enable flake8 #3224

hmaarrfk commented Jun 27, 2018 •

edited

Loading

This comment has been minimized.

codecov-io commented Jun 27, 2018 •

edited

Loading

hmaarrfk commented Sep 30, 2018

soupault Oct 2, 2018

hmaarrfk Oct 2, 2018

soupault Oct 2, 2018

hmaarrfk Oct 2, 2018

soupault Oct 2, 2018

hmaarrfk Oct 2, 2018

soupault Oct 2, 2018

hmaarrfk Oct 2, 2018

hmaarrfk commented Oct 2, 2018

soupault left a comment

hmaarrfk commented Feb 27, 2019

soupault commented Feb 27, 2019

Borda Apr 26, 2019

hmaarrfk Apr 26, 2019

Borda Apr 27, 2019

Borda Apr 26, 2019

hmaarrfk Apr 27, 2019

jni Apr 27, 2019

hmaarrfk Apr 27, 2019

Borda Apr 26, 2019

hmaarrfk Apr 27, 2019

Borda Apr 26, 2019

Borda Apr 26, 2019

jni Apr 27, 2019

hmaarrfk Apr 27, 2019

stefanv commented Dec 6, 2019

jni commented Dec 6, 2019

hmaarrfk commented Dec 7, 2019

stefanv commented Dec 7, 2019

grlee77 commented Oct 29, 2020

stefanv commented Feb 6, 2023

		@@ -134,10 +134,11 @@ def search_line(lines, refline, start=0):



		def getLutNames(prefix):
		def getLutNames(prefix, luts):

Enable flake8 #3224

Enable flake8 #3224

Conversation

hmaarrfk commented Jun 27, 2018 • edited Loading

For reviewers

This comment has been minimized.

codecov-io commented Jun 27, 2018 • edited Loading

Codecov Report

hmaarrfk commented Sep 30, 2018

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

Choose a reason for hiding this comment

hmaarrfk commented Oct 2, 2018

soupault left a comment

Choose a reason for hiding this comment

hmaarrfk commented Feb 27, 2019

soupault commented Feb 27, 2019

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

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

stefanv commented Dec 6, 2019

jni commented Dec 6, 2019

hmaarrfk commented Dec 7, 2019

stefanv commented Dec 7, 2019

grlee77 commented Oct 29, 2020

stefanv commented Feb 6, 2023

hmaarrfk commented Jun 27, 2018 •

edited

Loading

codecov-io commented Jun 27, 2018 •

edited

Loading