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

Run flake8 via pre-commit #891

Closed
wants to merge 5 commits into from
Closed

Run flake8 via pre-commit #891

wants to merge 5 commits into from

Conversation

jugmac00
Copy link
Member

@jugmac00 jugmac00 commented Sep 24, 2020

Similar to pyupgrade, also flake8 can and probably should be run via pre-commit.

e.g. tox -e pre-commit

This also fixes #826

This solution seems to be preferable over pinning versions in a requirements file, as here it is easier to update the dependencies - just run:
pre-commit autoupdate

e.g. `tox -e pre-commit`

modified:   .pre-commit-config.yaml
modified:   tox.ini
... as only `isort` is currently left.

modified:   tox.ini
@jugmac00 jugmac00 mentioned this pull request Sep 24, 2020
modified:   .pre-commit-config.yaml
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I additionally I'd expect a bit of documentation. I thought we already have a bit of developer documentation but I did not find it in the repository.

@jugmac00
Copy link
Member Author

Thanks for your feedback, @icemac

About the documentation - back when pyupgrade was added, I proposed to add some documentation to the tox env, see here
#859 (comment)

Zope/tox.ini

Lines 101 to 108 in 0402402

[testenv:pre-commit]
basepython = python3.6
description = This env runs all linters configured in .pre-commit-config.yaml
skip_install = true
deps =
pre-commit
commands =
pre-commit run --all-files --show-diff-on-failure

And yes, this page https://www.zope.org/developer/index.html needs an update. Let's make an issue for Monday's sprint?

I just had another look at my pr, and there is one problem with it.

As flake8 is now run via pre-commit, but pre-commit is not run via Travis, we have no more Flake8 check via Travis.

I suggest to add pre-commit via Travis, but to allow failures.

I hope this is both ok for you and for Jens:
#859 (comment)
#859 (comment)

@dataflake
Copy link
Member

I strongly dislike this. When we talked about pre-commit first in conjunction with pyupgrade we only put pyupgrade into pre-commit and left it out of tox so that something inconsequential will not make tox fail. Now you're moving flake8 in there as well which then requires running pre-commit on Travis so we get the flake8 warnings again. I would much prefer to keep the current tox setup and leave flake8 in there.

@dataflake
Copy link
Member

Correction: We left pre-commit out of Travis so Travis won't fail on unimportant stuff.

@dataflake dataflake self-requested a review September 25, 2020 08:14
Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a summary of my concerns:

You claim "flake8 can and probably should be run via pre-commit" without giving any valid arguments why. You also quote #826, but IMHO that was an unusual and rare situation. We have not seen it before, and we have not seen it again since. As I see it those are weak arguments in relation to the problems this patch will cause:

  • In order to still see flake8 failures in Travis you now must run pre-commit in Travis, which will also run pyupgrade. Add pyupgrade to linter setup #859 led to the introduction of pyupgrade and I argued that forcing pyupgrade runs on Travis will just add another barrier for contributors for no gain - both Jürgen and I agreed that we really don't care if people submit code that doesn't pass pyupgrade
  • you're also breaking the convention of running the linter and having linter configuration right inside tox.ini, and we use that convention in every single package in this organization that I am aware of. This is adding confusion.

In summary, I see no advantage here, just disadvantages. Please do not merge this.

@jugmac00
Copy link
Member Author

Please do not merge this.

No worries - I know this would be a major change, so I wanted to wait for the feedback of both of you.

The intention of this pr was...
Using pre-commit for all linters (also isort in future), tox only for running the tests and pre-commit, and then finally just run tox on travis would allow us to have super slick config files.

When we talked about pre-commit first in conjunction with pyupgrade we only put pyupgrade into pre-commit and left it out of tox so that something inconsequential will not make tox fail.

You certainly know that you can add tox envs, which are not run when you just enter tox - so in theory we could have added pyupgrade as a tox env, which then does not get listed in the envlist, just like autopep8 or isort-apply in the current tox.ini.

I proposed to use pre-commit as it does the heavy lifting of recursively finding all the files for the linter (which the previous manual attempt failed to do.). Additional, there are the advantages of pinning versions, easy update management, and the possibilty of adding a real pre-commit hook, so whoever wants (e.g. me) cannot commit something which would only fail on Travis (or if left out on Travis, fails at a core maintainer's tox run).

When we had the discussion about integrating pyupgrade there was the strong wish to not make it difficult for contributors, ie have a failure on CI when e.g. pyupgrade fails. CI currently fails when there is e.g. a superfluous empty line (via flake8). I do not see why an empty line is a good cause for a CI fail, but other linter warnings/errors are not. So this is entirely up to both of you to decide upon - and hopefully document one time.

And so we've come full circle. I do not think this pr is missing a documentation, but this project itself is missing an updated contributing documentation, ie https://www.zope.org/developer/index.html - and this is nothing an occasional contributor could do, but the core maintainers should do, in order to reflect your expecations.

Using pre-commit as described above would change some workflows, that is true. With https://github.com/zopefoundation/meta we would have the means to roll out changes to many repositories more easily. Lots of projects are using this new workflow (e.g. Flask ), but certainly there is no need to jump the bandwaggon (remember the black discussion). Although I was not part of it, there was a time when flake8 or tox was introduced and so changed the previous workflow.

Afaik, it is not possible to give different linters different weights with pre-commit, so this looks like a dead end. Feel free to close the pr - or - if interested, let's chat on Monday's sprint.

@dataflake
Copy link
Member

Using pre-commit for all linters (also isort in future), tox only for running the tests and pre-commit, and then finally just run tox on travis would allow us to have super slick config files.

That's an... interesting goal. Please don't forget that similar to coverage and .coveragerc this is a configuration in a dot file, so it's not obvious to many people. It will get overlooked. That's why I have been arguing to get rid of .coveragerc and store that configuration in setup.cfg. But that's yet another discussion, I just mention that as a side note.

When we talked about pre-commit first in conjunction with pyupgrade we only put pyupgrade into pre-commit and left it out of tox so that something inconsequential will not make tox fail.

You certainly know that you can add tox envs, which are not run when you just enter tox - so in theory we could have added pyupgrade as a tox env, which then does not get listed in the envlist, just like autopep8 or isort-apply in the current tox.ini.

I had sent a correction, what I meant was we left it out of Travis so Travis won't fail on something that's just not important.

I proposed to use pre-commit as it does the heavy lifting of recursively finding all the files for the linter (which the previous manual attempt failed to do.).

Not sure what you're referring to. We have well-working configurations for linters etc. and I am not aware that there's an issue with files getting overlooked.

When we had the discussion about integrating pyupgrade there was the strong wish to not make it difficult for contributors, ie have a failure on CI when e.g. pyupgrade fails. CI currently fails when there is e.g. a superfluous empty line (via flake8). I do not see why an empty line is a good cause for a CI fail, but other linter warnings/errors are not. So this is entirely up to both of you to decide upon - and hopefully document one time.

The difference between pyupgrade and flake8 is simple: I'd consider all output from pyupgrade as not important. flake8 is more important. Sure, it will also complain about unimportant stuff. But also about important stuff, unlike pyupgrade.

And so we've come full circle. I do not think this pr is missing a documentation, but this project itself is missing an updated contributing documentation, ie https://www.zope.org/developer/index.html - and this is nothing an occasional contributor could do, but the core maintainers should do, in order to reflect your expecations.

IMHO such a documentation effort would be a much better use of time than this PR. Or a much better use of time than e.g. upgrading isort and making all those cosmetic code changes.

Using pre-commit as described above would change some workflows, that is true. With https://github.com/zopefoundation/meta we would have the means to roll out changes to many repositories more easily.

There's a whole other discussion to be had about zopefoundation/meta and how useful it is. My gut feeling is that it's trying to do too much and imposing too many fixed policies that feel like a straightjacket in places, and some of those policies aren't a good fit for all packages. I have tried to use it early on but kind of gave up on it.

@jugmac00
Copy link
Member Author

As this issue has not been closed yet...

Django just announced they will use pre-commit.
https://github.com/django/django/blob/master/.pre-commit-config.yaml

Flask also uses pre-commit:
https://github.com/pallets/flask/blob/master/.pre-commit-config.yaml

Cherrypy uses pre-commit:
https://github.com/cherrypy/cherrypy/blob/master/.pre-commit-config.yaml

Morepath uses pre-commit:
https://github.com/morepath/morepath/blob/master/.pre-commit-config.yaml

@dataflake
Copy link
Member

I personally wouldn't consider "someone else is using it" a compelling argument.

@jugmac00
Copy link
Member Author

I added this PR to the sprint agenda of next Friday - either close it for good, or revive the discussion.

@icemac
Copy link
Member

icemac commented Apr 23, 2021

Decided to close during a meeting of the Zope April Sprint 2021.

@icemac icemac closed this Apr 23, 2021
@icemac icemac deleted the flake8-via-pre-commit branch April 23, 2021 07:15
@jugmac00 jugmac00 mentioned this pull request Apr 14, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pin flake8 and plugins
3 participants