rough flake8 integration #212

Merged
merged 1 commit into from Mar 10, 2015

Projects

None yet

2 participants

@collinanderson
Contributor

No description provided.

@bitprophet bitprophet and 1 other commented on an outdated diff Mar 5, 2015
dev-requirements.txt
@@ -7,5 +7,6 @@ alabaster>=0.6.0
nose==1.3.0
spec>=0.11.1
mock==1.0.1
+flake8
@bitprophet
bitprophet Mar 5, 2015 Member

Can you stick the latest stable release number in here as an ==? thanks!

@bitprophet bitprophet and 1 other commented on an outdated diff Mar 5, 2015
invoke/monkey.py
@@ -1,6 +1,9 @@
# Fuckin' A.
-import select, errno, os, sys
+import errno
@bitprophet
bitprophet Mar 5, 2015 Member

Unclear why this file is being touched, it's gone in HEAD. Can you try rebasing on latest HEAD perhaps?

@collinanderson
collinanderson Mar 5, 2015 Contributor

Ahh. Yup. I took too long making the change :)

@bitprophet bitprophet and 1 other commented on an outdated diff Mar 5, 2015
invoke/parser/parser.py
@@ -88,7 +88,7 @@ def parse_argv(self, argv):
# flag value and still in valid parsing territory (i.e. not in
# "unknown" state which implies store-only)
if not machine.waiting_for_flag_value and is_flag(token) \
- and not machine.result.unparsed:
+ and not machine.result.unparsed:
@bitprophet
bitprophet Mar 5, 2015 Member

Is 8-space indents on continuations a flake8 thing? It's not my preferred style :( (Honestly, I thought flake8 would complain about backslash continuations entirely, parentheticals are better anyway).

@collinanderson
collinanderson Mar 5, 2015 Contributor

It just doesn't want the conditional indented at the same level as the code.

@collinanderson
collinanderson Mar 5, 2015 Contributor

E129 visually indented line with same indent as next logical line

@collinanderson
collinanderson Mar 5, 2015 Contributor

This is now ignored under E125 for now.

@bitprophet bitprophet and 1 other commented on an outdated diff Mar 5, 2015
@@ -0,0 +1,4 @@
+[flake8]
+exclude=invoke/vendor,sites,tests
@bitprophet
bitprophet Mar 5, 2015 Member

Any particular reason to exclude tests?

@collinanderson
collinanderson Mar 5, 2015 Contributor

I force-pushed a change that now includes tests too. I just didn't want the diff to be too big :)

@bitprophet bitprophet and 1 other commented on an outdated diff Mar 5, 2015
@@ -0,0 +1,4 @@
+[flake8]
+exclude=invoke/vendor,sites,tests
+ignore=E124,E125,E128,E261,E302,E402,F401,W503,E712,F821
@bitprophet
bitprophet Mar 5, 2015 Member

I should look these up to see what they are, am not a linting expert.

@collinanderson
collinanderson Mar 5, 2015 Contributor

Yeah, I figure start with a lot of them ignored. If individual ones get fixed they can be removed from this list.

@collinanderson
collinanderson Mar 5, 2015 Contributor

Basically, you can remove one from the list, then run flake8 to see what lines they apply to.

@bitprophet
Member

Thanks for poking at this! See line comments :)

@bitprophet
Member

Also linking to #157 which this kinda supersedes.

@bitprophet
Member

Thanks for making those updates. Annoyingly, I didn't notice last time that there's apparently a rule for double spaces before line comments (e.g. foo = blah # lol) - this is another spot where I personally diverge from core PEP8. Sorry I didn't catch that earlier :(

Went and checked on all the stuff ignored/not ignored to see if I agree (I'd like to choose a single set of these rule ignores so I can apply them eventually to all of my projects :))...results:

  • As above, I'm thinking we ignore E261 (two spaces before line comments).
  • Why ignore F401 (module imported but unused)? When I run w/ it un-ignored I see 90% correct catches of unused imports (boy I should've put linting in before! thanks again for the PR...), with a handful of __init__.py convenience imports that we can presumably explicitly ignore.
  • I don't see W503 in pep8's docs, what's it do?
  • E711 and E712 - offhand I'd agree with those errors if they popped up. If I remove those from a copy of your branch, I see a handful of alerts and they all seem legit (where we use == instead of the arguably more correct is).
  • F811 and F821 also sound like rules I'd agree with if they alerted, and as above, if I remove them it does find some things I would consider errors :)
@collinanderson
Contributor

Don't worry, I added two space comments in the second change which is why you didn't catch it the first time. :)

I originally ignored all of the import cleanup just to keep the diff small enough. Here it is with all of the import cleanup.

@bitprophet bitprophet added the Support label Mar 10, 2015
@bitprophet bitprophet merged commit 83c633e into pyinvoke:master Mar 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bitprophet bitprophet added a commit that referenced this pull request Mar 10, 2015
@bitprophet bitprophet Changelog re #212 f2590e2
@bitprophet
Member

Looks good, thanks again! I might tweak that ignores list more in future but this is a great start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment