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

Add flake8 to pre-commit checks, with local bugbear as a plug-in #425

Closed
wants to merge 1 commit into from

Conversation

r-downing
Copy link
Contributor

@r-downing r-downing commented Nov 26, 2023

For #412

Considered doing this as a repo local-hook but it would require somehow hardcoding the venv location.

Implemented using pre-commit instance of flake8, and passing some additional config (and attr dependency) so that it can use the live bugbear.py from the repo.

This had to be split out to a separate config, because if it was in the base one, it would break local venv usage of flake8 because there would be duplicate bugbear plugins.

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Just want to get some confirmations and see if we can clean this up more ... Let's also see how CI goes ...

@@ -13,3 +13,11 @@ repos:
- id: black
args:
- --preview

- repo: https://github.com/pycqa/flake8
rev: 6.1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this rev refer to? Our latest tag / release is 23.9.16

If we can not use it cause re remove it or make it 99.0.0 or something + comment why we've done so

hooks:
- id: flake8
args: [--append-config=.flake8-pre-commit.cfg]
additional_dependencies: [attrs>=19.2.0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we need flake8 here? Can't our pyproject.toml handle this to avoid copy pasta / more things to change in the future?

- id: flake8
args: [--append-config=.flake8-pre-commit.cfg]
additional_dependencies: [attrs>=19.2.0]
exclude: ^tests\/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just so I understand, this is so we don't hit the test code that htis our checks on purpose right?

- id: flake8
args: [--append-config=.flake8-pre-commit.cfg]
additional_dependencies: [attrs>=19.2.0]
exclude: ^tests\/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just so I understand, this is so we don't hit the test code that htis our checks on purpose right?

@cooperlees
Copy link
Collaborator

O, my bad, this is for external users and not for running on CI runs? I'm super lost ... (can you tell I don't use pre-commit)

I tried to re-run the issue and PR, but still don't get what this solves / can be used for ... so if that could be explained that would be great too ... sorry, but no GitHub / precommit power user dur to working at a $bigCompany

@r-downing
Copy link
Contributor Author

O, my bad, this is for external users and not for running on CI runs? I'm super lost ... (can you tell I don't use pre-commit)

Hey, sorry if I didn't explain this very well! Yeah the goal is to add a flake8 check for pre-commit. Generally for pre-commit, you specify tool repos and revs (linters and checkers, etc) in .pre-commit-confg.yaml (that's the flake8 repo and rev 6.1.0), and then pre-commit installs self-contained versions of those tools in a cache directory to run during commit, etc.

So I was trying to add that flake8 check for this repo on pre-commit.

The tricky part is, that since it's a self-contained flake8 installation - how do we also run checks defined in our local plugin? We can specify flake8-bugbear as an additional dependency in the pre-commit-config, but then it's just pulled from pip, not the local one we're editing. Another option is pre-commit allows you to specify local hooks, but the drawback here is that it would be tied to the specific venv and installation location.

So the approach I came up with was to specify for pre-commit to use its own installation of flake8, and and pass in an arg for that additional config file so that it would pick up the bugbear plugin py file from within the repo. It's a little hacky, and I'm still troubleshooting it myself - seems to hang indefinitely on my PC if I run on all files. Started a draft because I kinda wanted to see if it worked in CI. Seemed to get picked up properly in the pre-commit CI at least.

@cooperlees
Copy link
Collaborator

cooperlees commented Nov 26, 2023

So, is your goal to just run flake8-bugbear via precommit? We do this with a project:

https://github.com/pypa/bandersnatch/blob/main/.pre-commit-config.yaml#L40

Isn't that what this does? What's the missing example with this approach I'm still not following?

  • I think you want to use bugbear.py from main / master / branch etc. right?

@r-downing
Copy link
Contributor Author

The goal was to run flake8-bugbear (the live one) on this project itself, so that new rules would apply in real-time

@r-downing
Copy link
Contributor Author

Something wonky about this config, probably more trouble than it's worth. Adding the basic checks here instead #427

@r-downing r-downing closed this Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants