MAINT: make jupyter-book conform to pep8 #126
Conversation
|
Woo, really nice, thanks. One potential other idea: how do you feel about using black (https://github.com/ambv/black) for code formatting? Basically as part of the PR process, black would be run on the PR just before merging into master. It's strictly Pep8 compliant and would mean nobody needs to worry about formatting reviews. |
|
Pining in a moderate opinion: I'm not a fan of Black -- we tried it in one of my projects where we have automatic linting through Travis and found that Black formatted code actually failed Flake8 linting 😱 That wouldn't be such a big deal if you could get every contributor to agree to using Black, but it also has some very specific stylistic preferences that I'm not a fan of. And unfortunately the styling comes "all or nothing," so you really have to adopt it across all of your files. Just my two cents, though ! |
|
I think that when it comes to "deciding whether to use a program that forcibly changes everybody's style to one highly opinionated thing", I'm fine with scrapping the idea if there's even a mild amount of unease with doing so...so I say we forget I suggested black and just use the tried-and-true method of pep8 :-) |
|
I'm also personally not a big fan of automated pep8. Just doing it here to reduce too much manual work in one go. I am of the opinion that you must write clean code to begin with. If you have a contributor who doesn't believe in writing pep8 compliant code, then the battle is already lost :p |
There was a problem hiding this comment.
This LGTM ! I had one question on whether or not linting should be its own test, but I'll leave it to you @choldgraf to decide if that's a separate PR :)
Thanks so much, @jasmainak ! 🚀
| @@ -89,6 +90,7 @@ jobs: | |||
| - run: | |||
| name: Tests | |||
| command: | | |||
| flake8 --count jupyter_book | |||
There was a problem hiding this comment.
One thought -- do we want this as just a count of errors or do we want a separate test ? The reason I ask is that if we want to ensure the codebase stays flake8 compliant, it's harder to remember to go check the circle logs than to just look for a quick pass / fail !
There was a problem hiding this comment.
It should fail if the count > 0. The reason is that an exit code of 0 means that it was successful. Maybe it's not so obvious? But it's what I've used in other repositories.
There was a problem hiding this comment.
Ah, I've never used it that way ! Very cool -- thanks for clarifying 😸
I guess my question then is if we should have the jupyter-book test fail with flake8 errors (rather than linting being its own test) since we might spin a few more cycles of Circle, as linting errors will need to be resolved before we can see functionality errors 😅 But I can really see it going either way !
There was a problem hiding this comment.
You mean the linting error should be thrown after the jupyter-book tests are run? I can change it to be that way if you prefer :) No strong preference either way. But with this approach, you can make a stronger case to make contributors conform to pep8 rather than waiting until the last commit to fix pep8. But yeah, both are fine by me !
|
I think we should give this a merge and decide on before/after tests once we've had a few cycles to play around with this setup. Thanks @jasmainak for the addition, I agree this is a big step to making jupyter-book a first-class coding project! |
cc @choldgraf
I fixed pep8 issues except the long lines (because I was lazy) using autopep8. While it's not fully ideal how autopep8 formats the code, conforming to pep8 will make it easier for new contributors to jump in.