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

Check pre-commit in GitHub Actions #626

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

chriszs
Copy link
Contributor

@chriszs chriszs commented Mar 10, 2024

This PR consolidates linting and type checking into a single step in CI which uses pre-commit. The benefit of this is that it enforces the use of pre-commit when submitting a PR, flagging when somebody has forgotten to do so and thus preventing the introduction of formatting issues into the code base (which will subsequently trip up anyone contributing to those files). This also upgrades mypy to 1.9.0 to clear an apparently erroneous pre-commit type checking error (error: Self? has no attribute "post" [attr-defined] when I ran pipenv run pre-commit run --all) and fixes formatting issues that have snuck in throughout the code base.

Closes #625

@chriszs chriszs force-pushed the 625-check-pre-commit-in-ci branch 2 times, most recently from f1cf118 to b3e7540 Compare March 22, 2024 18:57
@chriszs chriszs marked this pull request as draft March 22, 2024 20:03
@chriszs chriszs force-pushed the 625-check-pre-commit-in-ci branch 2 times, most recently from 8958f19 to 64c5fbe Compare March 23, 2024 08:24
@chriszs chriszs force-pushed the 625-check-pre-commit-in-ci branch from 64c5fbe to 37d5d90 Compare March 23, 2024 08:36
@chriszs
Copy link
Contributor Author

chriszs commented Mar 23, 2024

This was a relatively simple change when I first filed it, but it has gotten more complicated. Here's what I have had to do:

  • In response to some feedback from @stucka about type errors, I added type stubs for bs4 and openpyxl.
  • I then proceeded to modify the scrapers to fix the type errors, mostly by adding type guards.
  • I also modified the dependency for beautifulsoup4 from bs4, which according to the project description is a "a dummy package designed to prevent namesquatting on PyPI. You should install beautifulsoup4 instead." I believe type checking didn't work correctly until I did this, though I could have just been confused.
  • To prevent bs4 from upgrading to a beta release, I changed allow_prereleases in the Pipfile to false.
  • By this point a bunch of tests were failing in CI, so I pinned urllib3 to 1.26.18 to avoid an issue where vcrpy fails with urllib3 version 2.
  • I deleted and re-recorded the test cassettes from pytest-vcr to match updated package versions.
  • The Sphinx CI build was still failing, so I pinned a bunch of Sphinx-related deps to avoid another versioning issue.

These last couple of changes were not strictly related, but were effectively required to get CI to pass while installing packages because package versions are allowed to float, causing various package versioning mismatches.

@chriszs chriszs marked this pull request as ready for review March 23, 2024 08:54
@chriszs
Copy link
Contributor Author

chriszs commented Mar 25, 2024

Also updated the versions in pre-commit to match the versions in Pipfile.lock by using pre-commit autoupdate.

@stucka
Copy link
Contributor

stucka commented Mar 25, 2024

I'm speechless here, @chriszs . You are absolutely amazing.

Everything looks stellar, just stunning.

Just to double-check one thing right at the beginning -- you're replacing the old linting step with a call to run precommit, so there's no repetition?

Copy link
Contributor

@stucka stucka left a comment

Choose a reason for hiding this comment

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

Stunning work, @chriszs .

@stucka stucka merged commit b5a761e into biglocalnews:main Mar 25, 2024
10 checks passed
@chriszs
Copy link
Contributor Author

chriszs commented Mar 25, 2024

@stucka Yes, that's right.

@chriszs chriszs deleted the 625-check-pre-commit-in-ci branch March 25, 2024 14:24
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.

Check formatting with pre-commit in continuous integration GitHub Actions
2 participants