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 precommit in ci #3197

Closed
wants to merge 4 commits into from

Conversation

Lingepumpe
Copy link
Contributor

Currently running pre-commit produces quite a lot of (white space) changes in various .md and .yaml file. This is because
pre-commit hooks are not actively checked by CI and some contributors did not have them enabled. This PR fixes this:

  1. First all files are fixed by running pre-commit run --all-files
  2. https://github.com/pre-commit/action is added as step into the CI yaml. This should lead to github CI checks verifying the
    pre-commit rules are met.

@Lingepumpe Lingepumpe force-pushed the check_precommit_in_ci branch 2 times, most recently from b4618dd to 62c22a4 Compare April 18, 2023 07:05
@Lingepumpe
Copy link
Contributor Author

Rebased to current state of master branch

@Lingepumpe Lingepumpe force-pushed the check_precommit_in_ci branch 2 times, most recently from 0cc11be to f2acdec Compare April 21, 2023 06:26
hooks:
- id: isort
name: isort (python)
- repo: https://github.com/PyCQA/flake8
rev: "4.0.1"
rev: "5.0.4" # Newer version requires python-3.8 minimum
hooks:
Copy link
Collaborator

Choose a reason for hiding this comment

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

With all those versions, is there a possibility to tie them to whatever we have installed/pinned in the requirements-dev.txt and configured in the pyproject.toml?
Maybe by running pytest ${file} for all changed files that follow the flair/**/*.py pattern?
for example flake8 is actually restricted to "<5.0.0", so using 5.0.4 in the precommit is a mismatch.

for black this will surely lead to problems, when there is 24.* released, as then we have differen black versions fighting for formatting in the CICD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know a way to take the version from the requirements-dev.txt, I will adjust the flake8 version to match the requirements-dev.txt (to be <5.0.0).
Ideally you would adjust versions in both files simultaneously to match, this is not a new thing, this was previously also the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flake8 adjusted in pre-commit to most recent pre 5.0.0 version to match requirements-dev.txt => 4.0.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased on top of master

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know a way to take the version from the requirements-dev.txt

Like I suggested, run pytest ${file} on changed files that apply to the pattern flair/**/*.py.
I suppose a solution would look something similar to:

-   id: run linter
    entry: pytest
    pass_filenames: true
    files: ^flair/.*\.py$

Ideally you would adjust versions in both files simultaneously to match, this is not a new thing, this was previously also the case.

The previous case was different:
We never had percommit and tests synced, but precommit was never important as they weren't CI-tested. Now that you are adding them to the CI-tests, it is important that they are synced all the time.
Precommit has pinned versions, the requirements-dev.txt mostly unpinned versions, as we'd like to stay on the most actual version and rather make the active decision to not use it, if we see an issue.
Hence even if the versions are now the same, they will be out of sync as soon as a dependency gets updated.

Copy link
Contributor Author

@Lingepumpe Lingepumpe Apr 21, 2023

Choose a reason for hiding this comment

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

pytest cannot replace existing pre-commit hooks, as it does not edit any files, the point of pre-commit hooks like black, isort and the whitespace trimmers is that they change the files, pytest has tests such as pytest-black, but these only fail if the file is not black-formatted, they will not change the file.

Addtionally, running pytest as pre-commit hook is a bad idea, even if we restrict it to the flair folder: To run tests we want the full dependencies installed (e.g. mypy works better), running mypy (as part of pytest) in pre-commit environments is slow for a on-commit hook, and can lead to false positives when mypy runs without dependencies installed.

Concerning the "version drift" issue:
If requirements-dev.txt is not pinning the exact version, then yes, it could be that pytest will fail when such a linter is updated and adds a warning to code that previously passed. This was always the case, not new with this PR, it has nothing to do with pre-commit, only with pytest. For pre-commit: The versions are pinned in pre-commit-config.yaml, so if pre-commit passes once it should continue to pass, no matter what versions are released.

If you do update e.g. black on your dev machine, and it reformats the code in some new way, then try to commit, it will reformat it back via pre-commit (if you forgot to also update the black version in pre-commit-config). So you will notice immediately that you should also update pre-commit black version in this case of "incompatible formatting". This should be a rare case, as most changes in linters and code-formatters are backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent any version drift issues I have removed pytest-ing for isort, black and flake8, as it is covered by pre-commit, and we can avoid doing it twice (especially with potentially different versions).

@Lingepumpe Lingepumpe force-pushed the check_precommit_in_ci branch 3 times, most recently from 36af6d5 to cb8b418 Compare April 21, 2023 08:39
@Lingepumpe
Copy link
Contributor Author

Closing as the decision was made to rather drop pre-commit entirely

@Lingepumpe Lingepumpe closed this Apr 25, 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