[MRG + 1] CI some improvements to the flake8 CI #8036

jnothman · 2016-12-11T12:48:02Z

Examples now cannot fail due to 'E402 module level import not at top of file'
Changed files are checked for unused imports regardless of diff

In general, I find flake8 --diff somewhat deficient: it's not able to check for things like unused names. I think we therefore rely on it excessively.

Ideally we would:

flake8 over the old version
flake8 over the new version
use the diff to find corresponding line numbers between errors in old version and new version
report any error that does not correspond to an error in the old version

If anyone wants to implement that, they're welcome!

- Examples now cannot fail due to 'E402 module level import not at top of file' Changed files are checked for unused imports regardless of diff

lesteve

In general, I find flake8 --diff somewhat deficient: it's not able to check for things like unused names. I think we therefore rely on it excessively.

I agree it has shortcomings but it is still a lot better than nothing.

lesteve · 2016-12-12T11:08:58Z

build_tools/travis/flake8_diff.sh

+	echo "$files"
+	echo $options
+	git diff --unified=0 $COMMIT -- $files | flake8 --diff --show-source $options
+}

 if [[ "$MODIFIED_FILES" == "no_match" ]]; then
    echo "No file outside sklearn/externals and doc/sphinxext/sphinx_gallery has been modified"
 else
    # Conservative approach: diff without context so that code that


You want to move this comment before the git diff --unified=0 $COMMIT ... line. Basically this was an explanation why we use --unified=0 in the git diff command.

lesteve · 2016-12-12T11:10:25Z

build_tools/travis/flake8_diff.sh


 if [[ "$MODIFIED_FILES" == "no_match" ]]; then
    echo "No file outside sklearn/externals and doc/sphinxext/sphinx_gallery has been modified"
 else
    # Conservative approach: diff without context so that code that
    # was not changed does not create failures
-    git diff --unified=0 $COMMIT -- $MODIFIED_FILES | flake8 --diff --show-source
+	check_files "$(echo "$MODIFIED_FILES" | grep -v ^examples)"
+    # Examples are allowed to not have imports at top of file due to print(__doc__)


I would remove due to print(__doc__). This is not the main reason, this happens most of the time because examples are written in a "interactive mode" where you only import stuff when you need it rather than at the top of the file.

lesteve · 2016-12-12T11:11:39Z

build_tools/travis/flake8_diff.sh

+check_files() {
+	files="$1"
+	options="$2"
+	echo "$files"


Do we want to keep this debugging echo statements?

lesteve · 2016-12-12T11:12:48Z

build_tools/travis/flake8_diff.sh

+	check_files "$(echo "$MODIFIED_FILES" | grep -v ^examples)"
+    # Examples are allowed to not have imports at top of file due to print(__doc__)
+    check_files "$(echo "$MODIFIED_FILES" | grep ^examples)" --ignore=E402
+    check_files "$MODIFIED_FILES"


I don't get this last line ... You have already checked the non-examples files above and the example files ignoring E402.

Sorry, I basically fell asleep while testing this and left in a few debug statements! thanks for your patience!

lesteve · 2016-12-12T12:57:12Z

Ideally we would:
flake8 over the old version
flake8 over the new version
use the diff to find corresponding line numbers between errors in old version and new version
report any error that does not correspond to an error in the old version

matplotlib has a simplified version of it which checks (if I understood correctly) the number of flake8 errors by file and make sure that they do not go up. They are not all super happy with this though and at one point a PR was opened to copy the functionality of flake8_diff.sh: matplotlib/matplotlib#7403.

All in all I think this is a matter of 80/20 rule.

amueller · 2016-12-12T17:28:02Z

How about we error on all violations within a changed file? That'll reduce testing complexity at the cost of annoying committers for a little while.

Alternatively, we can can fix all pep8 violations (or mark them to be ignored by the linter if we don't like the fix) "once and for all" and then run flake8 on the changed files and not annoy future committers, but make everybody rebase their PRs once.

jnothman · 2016-12-13T00:52:25Z

It really shouldn't be that hard to do something that adjusts the line numbers to see new errors that have been introduced. I just don't have the capacity to focus on it right now...

…

On 13 December 2016 at 04:28, Andreas Mueller ***@***.***> wrote: How about we error on all violations within a changed file? That'll reduce testing complexity at the cost of annoying committers for a little while. Alternatively, we can can fix all pep8 violations (or mark them to be ignored by the linter if we don't like the fix) "once and for all" and then run flake8 on the changed files and not annoy future committers, but make everybody rebase their PRs once. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8036 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_IaJZLxQY8hq9NLB5Op1qvARqzSks5rHYQkgaJpZM4LJ6o7> .

amueller · 2016-12-13T00:53:04Z

but it's trying to solve a problem that will go away soon anyhow, right?

jnothman · 2016-12-13T01:11:13Z

how so?

…

On 13 December 2016 at 11:53, Andreas Mueller ***@***.***> wrote: but it's trying to solve a problem that will go away soon anyhow, right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8036 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz60lhtffkO6lQkT3d5Emj7fEmudXmks5rHexxgaJpZM4LJ6o7> .

amueller · 2016-12-13T01:15:11Z

well if we solve all pep8 in the codebase then we don't need to worry about checking only the diff, right?

jnothman · 2016-12-13T01:20:44Z

Do we have plans to solve all PEP8 in the codebase??

…

On 13 December 2016 at 12:15, Andreas Mueller ***@***.***> wrote: well if we solve all pep8 in the codebase then we don't need to worry about checking only the diff, right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8036 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz65HCrH9AQDP8-_WsME5fteafEE6Gks5rHfGggaJpZM4LJ6o7> .

amueller · 2016-12-13T01:28:38Z

Well if we only merge prs that don't introduce pep8 errors that will happen at some point, right? Unless we want to not enforce that and just use the continuous integration as a reminder.

jnothman · 2016-12-13T02:37:19Z

Only if the entire codebase is rewritten...?

…

On 13 December 2016 at 12:28, Andreas Mueller ***@***.***> wrote: Well if we only merge prs that don't introduce pep8 errors that will happen at some point, right? Unless we want to not enforce that and just use the continuous integration as a reminder. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8036 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6986vpc4x9pXGFTJar4furw4YJqAks5rHfTHgaJpZM4LJ6o7> .

8000

lesteve · 2016-12-13T08:29:25Z

I tested this locally and this works fine, so I am +1 for merging this.

I don't have a strong opinion about pep8-fying the full code but I think that can be discussed in a separate issue.

amueller · 2016-12-13T16:06:25Z

@jnothman https://erikbern.com/2016/12/05/the-half-life-of-code.html ;)

amueller · 2016-12-13T17:00:28Z

lgtm. Btw, can we please ignore W503? Do we have ignores setup somewhere? Or is our flake8 just old?

lesteve · 2016-12-13T17:08:21Z

Merging this one, thanks @jnothman!

jnothman · 2016-12-13T23:40:57Z

https://erikbern.com/2016/12/05/the-half-life-of-code.html ;)

I'm never convinced when measures are based on number of commits. Surely this is highly influenced by the projects' policies about commit atomicity. I wonder how robust those half lives are to filtering commits to, say, a minimum number of lines changed.

amueller · 2016-12-14T02:46:01Z

it's about how long it takes until a line changes, commits don't matter, I think.

Examples now cannot fail due to 'E402 module level import not at top of file'

CI some improvements to the flake8 CI

a9369ba

- Examples now cannot fail due to 'E402 module level import not at top of file' Changed files are checked for unused imports regardless of diff

jnothman added the Build / CI label Dec 11, 2016

lesteve reviewed Dec 12, 2016

View reviewed changes

jnothman and others added 2 commits December 12, 2016 23:04

Remove debug statements; clean up comments

0e91abb

COSMIT use spaces instead of tabs

655c1d9

lesteve changed the title ~~CI some improvements to the flake8 CI~~ [MRG + 1] CI some improvements to the flake8 CI Dec 13, 2016

lesteve merged commit aceb73c into scikit-learn:master Dec 13, 2016

amueller mentioned this pull request Dec 14, 2016

[MRG] K-SVD #3739

Closed

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG + 1] CI some improvements to the flake8 CI (scikit-learn#8036)

06396ef

Examples now cannot fail due to 'E402 module level import not at top of file'

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG + 1] CI some improvements to the flake8 CI (scikit-learn#8036)

799fc18

Examples now cannot fail due to 'E402 module level import not at top of file'

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG + 1] CI some improvements to the flake8 CI (scikit-learn#8036)

39f9dd1

Examples now cannot fail due to 'E402 module level import not at top of file'

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG + 1] CI some improvements to the flake8 CI (scikit-learn#8036)

44c464a

Examples now cannot fail due to 'E402 module level import not at top of file'

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG + 1] CI some improvements to the flake8 CI (scikit-learn#8036)

5a79a07

Examples now cannot fail due to 'E402 module level import not at top of file'

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG + 1] CI some improvements to the flake8 CI #8036

[MRG + 1] CI some improvements to the flake8 CI #8036

[MRG + 1] CI some improvements to the flake8 CI #8036

[MRG + 1] CI some improvements to the flake8 CI #8036

Conversation

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