Opened 5 years ago

Closed 5 years ago

#7631 closed enhancement (implemented)

Try a PEP8 style checker

Reported by: atagar Owned by: atagar
Priority: Low Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords: testing
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should look into using flake8 to do static analysis as part of our tests...

http://pypi.python.org/pypi/flake8/
http://dancallahan.info/journal/python-flake8/

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by atagar

  • Keywords controller added

comment:2 Changed 5 years ago by atagar

  • Keywords testing added; controller removed

Oops, wrong keyword.

comment:3 Changed 5 years ago by robinson

Damian,

Do you have a particular interest in flake8? Or will separate pep8 and PyFlakes work as well?

comment:4 Changed 5 years ago by atagar

  • Summary changed from Use flake8 for tests to Try a PEP8 style checker

Nope, I don't have any strong feelings about flake8. Actually, I've never used it before - the only tool I've used for static analysis of python is pylint. This ticket is just to evaluate a style checker to see if it's helpful or more hassle than it's worth.

Revising the ticket summary.

comment:5 Changed 5 years ago by robinson

I have used pep8[0] and I do not think you will like what it tells you about the Stem source code. PEP8 has whitespace specifications much different from the current Stem practice (e.g. indents should be four spaces, should be two blank lines above/below class definitions, etc.) To keep the hassle down, I recommend looking for something (not pep8) that allows configurable specs.

[0]: http://pypi.python.org/pypi/pep8

comment:6 Changed 5 years ago by atagar

  • Resolution set to wontfix
  • Status changed from new to closed

Gotcha. I wouldn't mind getting a little more in line with PEP8 if it makes collaboration easier, but there's some things like the two space indentation that I'd be very reluctant to change.

I'm gonna resolve this for now since we don't really have a good story for what we want from static analysis. Personally I find the output of pylint to be useful and reasonably configurable, but even it gets confused by core pieces of stem's codebase (like enums).

comment:7 follow-up: Changed 5 years ago by atagar

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Reopening. This is understandably a pain point for new contributors so we really should change it. I'd be fine with replacing our style checker [1] with something that gets us closer to PEP8 compliance. After all, it's easier for me to change my habits and vimrc than ask it of the world. ;)

My one caveat to PEP8 is the four space indentations. From what I've seen python projects very widely on this. If active contributors really want us to move to four spaces then fine, but I'd like to have some discussion around that first.

I'm neck deep in the arm migration at present so it'll be a while before I can get to this. If stem's style really annoys you then you're more than welcome to take the lead on this!

[1] https://gitweb.torproject.org/stem.git/blob/HEAD:/test/check_whitespace.py

comment:8 Changed 5 years ago by robinson

Damian,

I recently checked out pylint, pyflakes, flake8, and pychecker (?). None of them impressed me enough to want to use on any of my projects.

I looked into configuring pep8 for Stem and I think I was too harsh before. The --ignore flag does let one turn off specific error-checking.

pep8 --ignore=E111,E121 run_tests.py

I was able to disable "E111 indentation is not a multiple of four" and "E121 continuation line indentation is not a multiple of four" messages with the above command, which reduced the errors/warnings on run_tests.py from 295 to 120.

pep8-py2.7 --ignore=E111,E121,W293 run_tests.py

Adding a warning "W293 blank line contains whitespace" to the ignore list, gets down to 42 messages (mainly "E501 line too long").

While the current coding style is not what I'm used to, it's not unreasonable. It does mean you, Damian, get to clean up submissions from new contributors until we each figure things out.

comment:9 Changed 5 years ago by atagar

Thanks for looking into this, Sean! I'll try to look into swapping us over this next week (barring surprises).

comment:10 in reply to: ↑ 7 Changed 5 years ago by neena

Replying to atagar:

Reopening. This is understandably a pain point for new contributors so we really should change it. I'd be fine with replacing our style checker [1] with something that gets us closer to PEP8 compliance. After all, it's easier for me to change my habits and vimrc than ask it of the world. ;)

My one caveat to PEP8 is the four space indentations. From what I've seen python projects very widely on this.

By complying with pep8, we would be helping maintain consistency among them. :p

If active contributors really want us to move to four spaces then fine, but I'd like to have some discussion around that first.

I don't mind the current style. But, I think it would be better for Stem to comply with PEP8 completely simply because it's The Convention To Confirm To.

comment:11 Changed 5 years ago by atagar

  • Resolution set to implemented
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.