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

Update+apply pre-commit, prepare for pre-commit.ci, add badge #327

Merged
merged 4 commits into from
Feb 6, 2023

Conversation

dbast
Copy link
Member

@dbast dbast commented Feb 3, 2023

Description

Update+apply pre-commit hooks (isort requires an update to work again).

This also prepares for the usage of pre-commit.ci:

  • Usually faster than the pre-commit Github Action with Cache
  • Enabled also for several other repos in the conda organisation
  • Shifts some CI load off from Github Actions (CI in the conda org sometimes is clogged due many conda/conda-build PRs with their huge/long-running Testsuites)
  • pre-commit.ci also has an autofix feature which is disable in the config instead commenting "pre-commit.ci autofix" in a PR is enough.

I am (as member of the @conda/infrastucture team) happy to enable pre-commit.ci for this repo, if the @conda/conda-lock agrees to the idea of using pre-commit.ci. That said, I am also fine with descoping the PR to updates only and sticking to GH Actions.

@dbast dbast requested a review from a team February 3, 2023 17:36
Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Generally I prefer the transparency of GHA, but for the case of pre-commit, I agree that it probably makes more sense to use pre-commit.ci.

Also, we should probably merge this sooner rather than later to get the isort fix.

@dbast
Copy link
Member Author

dbast commented Feb 4, 2023

Ok. Going to enable pre-commit.ci for the repo (and changing the required status checks) ... and close / re-open the PR should then show that it works.

@dbast dbast closed this Feb 4, 2023
@dbast dbast reopened this Feb 4, 2023
dbast added 2 commits February 4, 2023 08:43
tests/conftest.py: error: Source file found twice under different module names: "conftest" and "tests.conftest"
tests/conftest.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
tests/conftest.py: note: Common resolutions include: a) adding `__init__.py` somewhere, b) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)
@dbast
Copy link
Member Author

dbast commented Feb 4, 2023

Had to fix the error:

tests/conftest.py: error: Source file found twice under different module names: "conftest" and "tests.conftest"
tests/conftest.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
tests/conftest.py: note: Common resolutions include: a) adding `__init__.py` somewhere, b) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)

Added a __init__.py to the tests folder ... this seems to have triggered mypy to scan files it previously didn't scan, due to pre-commit being configured to only scan changed files... so I had to exclude tests/test_conda_lock.py from mypy as currently 73 errors.

see also python/mypy#4008

This is now green on pre-commit.ci and locally with pre-commit run --all (which wasn't the case before).

@maresb
Copy link
Contributor

maresb commented Feb 4, 2023

Thanks a lot for this! All green now. I noticed one more local mypy error on my side, corrected in dbast#2

@maresb
Copy link
Contributor

maresb commented Feb 4, 2023

Now that CI has finished, I'll merge it in this branch...

@maresb
Copy link
Contributor

maresb commented Feb 4, 2023

ping @mariusvniekerk

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.

3 participants