Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Flake8 3.0.1 #1621

Closed
wants to merge 4 commits into from
Closed

Conversation

cbowman0
Copy link
Member

This exceeds hilarious. Opening just for discussion sake for now.

I ran stuff through autopep8 which eliminated most of the noise from the newer flake8. Manually ignored some errors for now, since fixing them isn't easy.

The pep8 statistics from current master are:

/git/graphite-web/webapp/graphite$ pep8 --statistics --quiet --filename=*.py *.py */*.py 
app_settings.py
carbonlink.py
intervals.py
logger.py
middleware.py
node.py
readers.py
remote_storage.py
settings.py
storage.py
util.py
views.py
wsgi.py
account/admin.py
account/ldapBackend.py
account/models.py
account/views.py
browser/views.py
composer/views.py
dashboard/models.py
dashboard/send_graph.py
dashboard/views.py
events/models.py
events/views.py
finders/ceres.py
finders/__init__.py
finders/standard.py
metrics/views.py
render/attime.py
render/datalib.py
render/evaluator.py
render/functions.py
render/glyph.py
render/grammar.py
render/hashing.py
render/views.py
url_shortener/baseconv.py
url_shortener/models.py
version/views.py
whitelist/views.py
2905    E111 indentation is not a multiple of four
133     E121 continuation line indentation is not a multiple of four
1       E122 continuation line missing indentation or outdented
1       E124 closing bracket does not match visual indentation
1       E125 continuation line does not distinguish itself from next logical line
2       E126 continuation line over-indented for hanging indent
4       E127 continuation line over-indented for visual indent
18      E128 continuation line under-indented for visual indent
281     E201 whitespace after '('
280     E202 whitespace before ')'
164     E203 whitespace before ':'
13      E221 multiple spaces before operator
6       E222 multiple spaces after operator
20      E225 missing whitespace around operator
1       E227 missing whitespace around bitwise or shift operator
2       E228 missing whitespace around modulo operator
536     E231 missing whitespace after ','
20      E251 unexpected spaces around keyword / parameter equals
93      E261 at least two spaces before inline comment
53      E262 inline comment should start with '# '
1       E271 multiple spaces after keyword
1       E272 multiple spaces before keyword
1       E301 expected 1 blank line, found 0
172     E302 expected 2 blank lines, found 1
18      E303 too many blank lines (3)
4       E401 multiple imports on one line
507     E501 line too long (82 > 79 characters)
14      E502 the backslash is redundant between brackets
52      E701 multiple statements on one line (colon)
4       E703 statement ends with a semicolon

On this branch:

/git/graphite-web/webapp/graphite$ pep8 --statistics --quiet --filename=*.py *.py 
carbonlink.py
intervals.py
logger.py
middleware.py
readers.py
remote_storage.py
settings.py
storage.py
util.py
93      E501 line too long (83 > 79 characters)

All programmatic tests ran clean. There are 2 lint errors in the test files:

/git/graphite-web/webapp/tests/test_util.py:14:0: F401 'graphite.wsgi.application' imported but unused
/git/graphite-web/webapp/tests/test_functions.py:2110:8: F841 local variable 'results' is assigned to but never used

The first is from an import that has a # NOQA comment on it. Thoughts on a fix?
The second is because the check doesn't have an assert on the result value. So, that's an oops. I just need time to determine what the check should be.

Set flake8 to ignore E501 (long lines)

Ran the following on graphite/webapp/

autopep8 . --ignore=E501 --recursive --in-place --pep8-passes 2000 --verbose
* ignore E402,E731 and F841

* Slight indent changes

* Revert logic from if not <var> in <thing> to if <var> not in <thing>
Set flake8 to ignore E501 on the tests subdir

Ran the following on webapp/tests/

    autopep8 . --ignore=E501 --recursive --in-place --pep8-passes 2000 --verbose
@obfuscurity
Copy link
Member

This doesn't surprise me at all since we've never enforced pep8 indentation. I'll try and review later tonight but it would be good to have some extra sets of 👀 on this.

/cc @graphite-project/committers

@JeanFred
Copy link
Member

Thanks for this − not a fun patch to make! :)

I’d suggest going through this in two passes − one ignoring E111, and one only fixing E111. Otherwise the diff makes it insane to spot issues (although autopep8 should be fairly safe to use).

P.S. My slightly preferred way to control flake8 ignores is via a dedicated entry in tox.ini − not that it matters much :)

[flake8]
exclude = .venv,.tox
ignore = E111,E231
# E111 indentation is not a multiple of four
# E231 missing whitespace after ','

@cbowman0
Copy link
Member Author

Splitting into multiple pull requests can be done. I'll attempt it this morning.

I did attempt to add a [flake8] section to tox.ini but was unsuccessful in having it affect flake8 runs.

@brutasse
Copy link
Member

I usually put a [flake8] section in setup.cfg. But other locations seem to be supported… http://flake8.pycqa.org/en/latest/user/configuration.html#configuration-locations

@cbowman0
Copy link
Member Author

Turns out that's where the [flake8] is defined in graphite. https://github.com/graphite-project/graphite-web/blob/master/setup.cfg#L19

By the looks of things, it isn't being respected.

@cbowman0
Copy link
Member Author

Here's the bug in flake8 https://gitlab.com/pycqa/flake8/issues/181 Fixed in 3.0.2 which was released last night.

Since we're already headed down the path of being pep8 compliant let's keep going for now.

@SEJeff
Copy link
Member

SEJeff commented Jul 27, 2016

Whoa we are ok with supporting general Python coding standards?! One of the things that always made me sad hacking on graphite was the very anti-pythonic coding style. I'm +1000 on any patches which make graphite project code look like the overwhelming majority of Python projects you see out there.

@cbowman0
Copy link
Member Author

Yup. Looks like it. Consistency is good.

I opened #1623 for a more staged approach at converting the code base to pep8 compliance. Please review that one.

I'm closing this without merging as it accomplished what was intended.

@cbowman0 cbowman0 closed this Jul 27, 2016
@brutasse
Copy link
Member

The only issue is that so much code changes that all currently open pull requests will have to be rebased or redone. I think that was the main counter-argument so far, not opposition to adopting more widely used standards.

@JeanFred
Copy link
Member

That is a good point − did not think of that.

Given how @obfuscurity is on a spree to tackle open pull-requests, might be a good plan to wait for the backlog to further slim down.

@obfuscurity
Copy link
Member

That might be a good idea. Although most of them seem to require rebasing anyways, I think it would make it a fair bit more challenging to throw that into the mix.

@cbowman0
Copy link
Member Author

I'm of the opinion that we make the change now. We have the most test coverage we ever have and the backlog is as small as I've ever seen it.

@mleinart
Copy link
Member

Note giant style changes like this will break git blame and make merges and
cherry picks (e.g. porting any new patches from 0.9.x to master) difficult
or impossible.

I'll encourage a re-reading of pep8's first section... but I'm going to let
go. You all do what you feel

On Jul 27, 2016 7:21 AM, "Christopher Bowman" notifications@github.com
wrote:

I'm of the opinion that we make the change now. We have the most test
coverage we ever have and the backlog is as small as I've ever seen it.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#1621 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATaL7_B-aIYA3smQIxC9rn_T06KLhkeks5qZ003gaJpZM4JVkNC
.

@SEJeff
Copy link
Member

SEJeff commented Jul 27, 2016

And a lot of the old 1-2 year old PRs likely never will be debased unfortunately.

@obfuscurity
Copy link
Member

Not true, I've been doing this work myself.

Jason Dixon
Sent from my iPhone

On Jul 27, 2016, at 8:37 AM, Jeff Schroeder notifications@github.com wrote:

And a lot of the old 1-2 year old PRs likely never will be debased unfortunately.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@obfuscurity
Copy link
Member

My vote would be to hold off until we can get all the PRs in milestone 1.0.0 rebased and merged. I'm 💯 on the spirit of this change, but I also dread the notion of losing git blame.

@obfuscurity
Copy link
Member

P.S. At least until we reach the 1.0.0 milestone. 😉

@cbowman0 cbowman0 deleted the flake8-3.0.1 branch August 18, 2016 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants