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

Addressing PEP8 by including pytest-flake8 #3944

Closed
noviluni opened this issue Aug 7, 2019 · 5 comments
Closed

Addressing PEP8 by including pytest-flake8 #3944

noviluni opened this issue Aug 7, 2019 · 5 comments

Comments

@noviluni
Copy link
Member

@noviluni noviluni commented Aug 7, 2019

As mentioned in the Scrapy Coding Style section in the docs (https://docs.scrapy.org/en/master/contributing.html#coding-style) :

“Unless otherwise specified, follow PEP 8.”.

However, it’s known that the current codebase has a lot of parts that don’t respect PEP8 guidelines (#2144).

I know that it’s difficult to address that, as it involves a lot of code to be reviewed but, however, the more time we skip that task the harder it becomes.

Two obvious things that we could do are:

  • Fixing the current code
  • Adding a pipeline to check pep8

Fixing the current code can’t be done in just one commit, but instead of fixing pep8 in specific project parts (example: #2207, #2429), it could better and easier to review if we fix rules one by one (for example: "Unused imports", "Missing blank lines after class or function definition."...).

My proposal to avoid to write a hard to maintain pipeline is to include the pytest-flake8 package that executes flake8 when executing pytest (by adding the --flake8 flag) and raises an error when the check fails.

If we start excluding all the rules then we can just fix one by one from more obvious rules to more discussed rules just excluding that rule from the ignored rules list. In that way we ensure that new code respects the previous agreed flake8/pep8 rules and on the other hand we can discuss every rule if we want to follow it or not (example: #3697).

I will add a PR to illustrate this idea.

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 7, 2019

What do you think?? I’ve been evaluating other possible solutions and I’ve seen that scikit-learn uses a pipeline to check flake8 against the new/diff lines with a bash script https://github.com/scikit-learn/scikit-learn/blob/master/build_tools/circle/flake8_diff.sh but it seems really hard to mantain.

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Aug 8, 2019

Maybe we could have a separate tox environment for these analysis? (#3727 could also be run there)

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 8, 2019

Hi @Gallaecio . Sounds good, I will try to update my PR when I have time.

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 9, 2019

Hi @Gallaecio I've updated the PR (#3945) with a new tox environment and a its travis pipeline and it seems that worked. Feel free to review it and give your feedback :)

@noviluni
Copy link
Member Author

@noviluni noviluni commented Nov 15, 2019

The PR (#3945) has been already merged so I proceed closing this. Thank you @Gallaecio 🎉 😄

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

No branches or pull requests

2 participants