Add pre-commit config, formatting with black, linting with ruff #2503
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think the CI failure is the same as this one: #2502 (comment) |
|
I just merged your other PR. Feel free to rebase/merge with |
Fix and update lint/format config
djhoese
pushed a commit
that referenced
this pull request
Jun 12, 2024
I think I've sorted out the problem causing #2596. Sorry this is a lot to take in. I'm just trying to be thorough and keep track for my own purposes as well. Before I write any more I'll try to be clear: * _tests_ are the individual test functions in the test suite * _checks_ are the the output of GitHub actions workflows * _steps_ are the individual steps in a workflow * _commands_ are the individual lines in a `run` step ### Tests The issue was that the GitHub actions workflow is replacing the default shell with `bash -l {0}`. https://github.com/vispy/vispy/blob/4f061ba23ddac3131eb0d1d955573dd71c9a3d2f/.github/workflows/main.yml#L163-L166 This is problematic because it does not fail a run step if a command fails. I've changed the shell to `bash --login -e {0}` which will fail the run step if any command fails. The default shell on GitHub actions sets `-e`, and I think it's what we want. Without this, the step (and check) will only fail if the _last_ command in the step fails. I didn't test, but I believe the login shell may be required for some conda things. Many checks would fail not directly because the test failed, but because the subsequent coverage commands would fail (because the tests failed in a way that didn't produce coverage). https://github.com/vispy/vispy/blob/4f061ba23ddac3131eb0d1d955573dd71c9a3d2f/.github/workflows/main.yml#L274-L275 Because of all this, I think the only tests that would actually be missed were any marked `@requires_pyopengl()` but not `@requires_application()` (as the in the case in #2589). Documenting the process in this PR: * See this log showing a check that passed, but should have failed (5964e1e): https://github.com/aganders3/vispy/actions/runs/9388966425/job/25855466353#step:11:647 * See here for a check that failed as expected, after changing the shell (8a314d2): https://github.com/aganders3/vispy/actions/runs/9389260260/job/25856378619?pr=6#step:11:647 The remaining failing test should be resolved by #2595. Fortunately it seems there are no additional missed failing tests, this may be luck but let's chalk it up to rigor! I think it's been this way for a long time. ### Linting Linting errors have been ignored for a totally different reason. The way the linting is set up, it invokes `flake8` by importing and running its `main` function. This is not officially supported, and there was a breaking change introduced in flake8 v5.0.0. The `main` function now returns an exit code instead of exiting (`sys.exit` or `raise SystemExit`). This is also fixed here, but given there are a number of linting errors I've deliberately disabled the check. I can open another PR to fix the lint errors if there is bandwidth to review it (or I can revive [my PR that blackens the code](#2503)). There was some discussion these were deliberately disabled before, but I think that was only the docstring checks: https://github.com/vispy/vispy/blob/4f061ba23ddac3131eb0d1d955573dd71c9a3d2f/.github/workflows/main.yml#L70-L73 Linting process in this PR: * See here for linting errors properly failing (fdb78e0): https://github.com/aganders3/vispy/actions/runs/9389579018/job/25857384162 * See here for linting errors reported but deliberately ignored (bd2a299): https://github.com/aganders3/vispy/actions/runs/9389634299/job/25857588048
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This closes #2291 and fixes the broken lint check in CI (#2291 (comment)) by replacing it with pre-commit. This will be a lot to review but hopefully it helps give a sense of the scale of changes. Timing-wise I'm not sure if there are any existing PRs you'd rather merge first (I have two open that would need to be rebased, haha).
Configuration is almost entirely in
pyproject.toml, with a bit in.pre-commit-config.yaml. The main exclusion isvispy/gloo/gl, which I believe is all/mostly generated code. I tried to comment the configs to explain how values, but of course I'm open to changes on that as well. I played with line-length a bit to pick a sane-ish value and settled on 100 for black, with ruff errors at 110. Pulling it down to 88 (black's default) or 79 created over 1000 more errors I think. So, a lot more but maybe a drop in the bucket compared to the changes already here.Finally, this adds a
.git-blame-ignore-revsfile so the one massive commit would be skipped by default in "blame" view on GitHub.