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

automation of linting test using pre-commit #11205

Open
12rambau opened this issue Feb 19, 2023 · 8 comments · May be fixed by #11603
Open

automation of linting test using pre-commit #11205

12rambau opened this issue Feb 19, 2023 · 8 comments · May be fixed by #11603

Comments

@12rambau
Copy link

I was working on a PR and I was surprised to see many linting errors.
I didn't see any documentation on which test I should run from my side (ruff, flak8 isort others).

Describe the solution you'd like
Use pre-commits to run all these linting checks automatically and use the same pre-commits in the github tests for consistency

@hugovk
Copy link
Contributor

hugovk commented Feb 21, 2023

Another benefit is pinning the lint tools, so you don't get hit by sudden failures when linters release with add new checks.

@onerandomusername
Copy link

Another benefit is pinning the lint tools, so you don't get hit by sudden failures when linters release with add new checks.

Yeah, I noticed that ruff is unpinned and will install whatever the latest version is. That's going to cause a lot of errors with how often ruff has a release with new features.

There's over 100 lint failures on master right now.

@12rambau
Copy link
Author

I started to work on it with the combo: black/prettier/ruff and it works quite ok.
Problem is taht lots of tests are now failing because of the linting. That make the test non robust to lint which is (according to me) an issue, I'll see if adding "pytest-regressions" and "beatifulsoup" could save us

@hugovk
Copy link
Contributor

hugovk commented Feb 28, 2023

As a first step I suggest pinning to the linter versions that last passed, to get the infrastructure in place.

Then we can update the versions and fix new warnings in a controlled manner in followups.

@jdillard
Copy link
Contributor

Then we can update the versions and fix new warnings in a controlled manner in followups.

Is there a "best practice" for doing this? I wasn't sure if it is just setting a reminder or maybe a cron job that runs the latest version so you can monitor the number of new warnings and fix them before they get out of hand.

@12rambau
Copy link
Author

You can but do you really want to ? ruff and black are sending reminder in the pre-commit commands but to be fair once you set up something that works and more importantly that is consistent, you don't want to change it every month (unless you identify an error or want to add a new feature)

@hugovk
Copy link
Contributor

hugovk commented Feb 28, 2023

I use https://pre-commit.ci - it autofixes PRs, and sends autoupdate PRs every month (by default, but I set it to quarterly with this at the end of .pre-commit-config.yaml:

ci:
  autoupdate_schedule: quarterly

Also doesn't stop me running pre-commit autoupdate, which I might do if I'm also updating other stuff in the config file.

@12rambau 12rambau mentioned this issue Mar 19, 2023
4 tasks
@12rambau
Copy link
Author

I'll go step by step as all the linting operation will crash. step one black + pre-commit + github actions

@AA-Turner AA-Turner added this to the some future version milestone Apr 29, 2023
@stephenfin stephenfin linked a pull request Aug 16, 2023 that will close this issue
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 a pull request may close this issue.

5 participants