Make it easier to run flake8 for contributors

[sirosen]

For an external contributor who drops by, it's not immediately obvious what the linting/testing expectations are. Testing is harder and more complicated, but I think linting could be smoothed out. For now, just looking at the flake8 usage: two easier options than the current flow.

1. Add flake8 to pre-commit config
2. Setup a tox.ini which allows for tox -e lint or similar to run linting

There are, of course, other options too, but these two are my preferred ones. The goal is to make installing flake8, flake8-pyi, and any other flake8 plugins a managed process via some tool.

Would either of these additions be welcome? If flake8 goes into pre-commit, then pre-commit.ci takes care of running it and perhaps it can be removed from GitHub actions.

[AlexWaygood]

Speaking personally: I have no objection to adding flake8 to pre-commit.ci, but I'm not sure I see the benefit relative to our existing setup :)

The goal is to make installing flake8, flake8-pyi, and any other flake8 plugins a managed process via some tool.

Can't you just install them all at once by doing pip install -r requirements-tests.txt?

I've never used tox, so can't comment there.

[sirosen]

Can't you just install them all at once by doing pip install -r requirements-tests.txt?

Yes, and I don't think this is bad. I just think we can do even better. 😄

if you're not familiar with the project, I think it's not obvious that you need to do this. The contrib doc is great, but it covers testing, with no explicit mention of flake8, and says to run the formatters with pre-commit. I think the way I knew what to do was actually by opening up the CI config to see what was done there.

I've never used tox, so can't comment there.

In that case, I'd be hesitant to introduce it just for this purpose.

I think tox is fabulous. But it's really at its best when you want to use it to make all of your CI steps runnable locally. If we aimed for tox -e lint, tox -e pytype, tox -e mypy, tox -e pyright, and tox -e stubtest-third-party as commands, I think it could deliver significant value.

[srittau]

I haven't use tox, either, although having a tox configuration seems reasonable. That said, I would want at least one active maintainer to have some experience with it, otherwise I fear the config might get neglected when we make changes.

[AlexWaygood]

and says to run the formatters with pre-commit.

Heh, I don't actually have pre-commit installed locally either — I just run black and isort manually if I remember, and rely on the pre-commit bot in our CI to correct any formatting sins if I forget.

[AlexWaygood]

Does #7992 help clarify the expectations for contributors a bit?

Specifically, for me: I have no expectations, I don't look down on a PR at all if it fails CI at first :) I feel quite strongly that running the tests before filing a PR should be completely optional for typeshed.

(To be clear, I'm not filing a docs PR because I think we shouldn't add a tox config or whatever -- we can do that as well! Though I agree with @srittau that it would be probably be good to have someone on the team who knows something about tox if we're going to do that.)

[srittau]

We are using tox for typing_extensions, so maybe @JelleZijlstra has some experience.

[sirosen]

Does #7992 help clarify the expectations for contributors a bit?

Yep, gave it a 👍 ! I think just being able to open the doc and /flake8/Ctrl+F: flake8 will make it easier to find.
My hope would be that with more config we can simplify that doc a bit.

Specifically, for me: I have no expectations, I don't look down on a PR at all if it fails CI at first :) I feel quite strongly that running the tests before filing a PR should be completely optional for typeshed.

I think this is the right attitude, and don't mean to push for anything else. But I keep making PRs which fail flake8, and I'd like to make it easier to make sure they don't. Maybe that's an argument for pre-commit config.

As the requester, I'd be most happy with (and willing to put together a PR for) any of these options:

1. pre-commit config adds flake8
2. new tox config with tox -e lint running pre-commit run -a; flake8 .
3. pre-commit config for flake8, and tox -e lint config to run pre-commit run -a (⭐ my favorite)

(1) is the lowest impact in terms of maintainers learning new tools, (2) probably has the lowest number of new physical lines added to the repo because CI is left untouched, (3) gives (IMO) the best developer experience with the most options.

If there's another idea to put on the table, I'm happy to consider. Or I'm also okay with being told the maintainers are satisfied with the current config and don't want to make changes.

[Akuli]

I don't like tox. It just doesn't solve a problem that I have, and it feels like an unnecessary layer of abstraction for running commands. After all, most of the time we just want GitHub Actions to run some commands to check a PR, and we don't want the contributor to run anything locally at all.

I'm fine with adding flake8 to pre-commit. As far as I can tell, the only disadvantage is what we see when a PR fails flake8: currently GitHub Actions shows it as flake8 failing, but with flake8 in pre-commit it would show up as pre-commit.ci failing.

[srittau]

I rather prefer to have a separate flake8 step that having it mashed in with pre-commit. In my mind, we should use pre-commit to autofix autofixable issues, but nothing else.

[sirosen]

It sounds like tox config would be an unwelcome addition, rather than a welcome one, and there's a preference for running flake8 as a separate CI step. I disagree about tox, but am happy to consider that part of the discussion closed.

I can see the merit in having flake8 separate in CI. It could be added to pre-commit for local hooks and marked in .pre-commit-config.yaml under ci.skip, so that pre-commit.ci does not run it. Would that be more interesting?

In my mind, we should use pre-commit to autofix autofixable issues, but nothing else.

Is there a specific reason for this? I noticed that the config here is very bare -- which is great in that it keeps the hooks very fast -- but it's very different from the way I see pre-commit commonly used. My own repos, and those I've seen in other projects using pre-commit, tend to be running a larger selection of checkers and fixers. e.g. I rarely see a repo not using any of the pre-commit-hooks hooks.

I'm trying to see if there's room for a change to run flake8 as a hook, disabled in CI. Then black, isort, and flake8 invocation can be covered in the contrib doc by a short section explaining how to run pre-commit run -a.

[srittau]

I can see the merit in having flake8 separate in CI. It could be added to pre-commit for local hooks and marked in .pre-commit-config.yaml under ci.skip, so that pre-commit.ci does not run it. Would that be more interesting?

Sounds good to me.

In my mind, we should use pre-commit to autofix autofixable issues, but nothing else.

Is there a specific reason for this?

This makes it easier to see what step failed from GitHub's CI UI. It also makes maintenance easier, as we don't have to deal with pre-commit when we want to change how our flake8 tests are run. pre-commit just adds unnecessary overhead here.

[AlexWaygood]

In my mind, we should use pre-commit to autofix autofixable issues, but nothing else.

Is there a specific reason for this? I noticed that the config here is very bare -- which is great in that it keeps the hooks very fast -- but it's very different from the way I see pre-commit commonly used. My own repos, and those I've seen in other projects using pre-commit, tend to be running a larger selection of checkers and fixers. e.g. I rarely see a repo not using any of the pre-commit-hooks hooks.

To understand how we're thinking about pre-commit, it might be helpful to go back to the reason why we introduced pre-commit to this repo a few months ago. We primarily introduced pre-commit for the "auto-pushing to PR branches" feature rather than to enable the use of local pre-commit hooks. We love black and isort, but they were getting in the way of contributors. Typeshed contributors often file PRs by editing files directly in the GitHub web UI and sometimes don't even have a local clone. For these contributors, it was a massive pain having black and isort fail in CI, and it's far more ergonomic just to have those issues autofixed for them after the PR is filed.

So, I've got nothing against contributors using pre-commit hooks locally if it's something they'd like to use, but it was never the primary reason we introduced pre-commit to the repo, and it's not something I've personally ever felt the need for with typeshed :)

But if there's a way of running flake8 as a hook locally, while still keeping our CI check separate, that sounds fine by me!

[AlexWaygood]

I'm trying to see if there's room for a change to run flake8 as a hook, disabled in CI.

I think we all agree that we'd be happy to accept a PR along these lines, @sirosen :)

If not, however, I think we should probably close this issue, as I doubt anybody on the maintainer team is going to file a PR like this.