-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add GitHub workflow for running pre-commit #51
Conversation
This commit updates the Python scripts based on the automatic fixes applied using the ruff configuration introduced in the subsequent commit: ``` $ pre-commit run -a [...] Fixed 85 errors: - Modules/Scripted/Home/Home.py: 3 × COM812 (missing-trailing-comma) 2 × D202 (no-blank-line-after-function) 1 × UP015 (redundant-open-modes) 1 × UP004 (useless-object-inheritance) - Modules/Scripted/VPAWModel/VPAWModel.py: 31 × COM812 (missing-trailing-comma) 2 × D202 (no-blank-line-after-function) 1 × F401 (unused-import) - Modules/Scripted/VPAWVisualize/VPAWVisualize.py: 32 × COM812 (missing-trailing-comma) 4 × RUF010 (explicit-f-string-type-conversion) 1 × SIM114 (if-with-same-arms) - Modules/Scripted/VPAWVisualize/vpawvisualizelib/isosurfaces.py: 6 × COM812 (missing-trailing-comma) 1 × D202 (no-blank-line-after-function) ```
This commit updates the markdown files based on the automatic fixes applied using the prettier introduced in the subsequent commit.
Once this is integrated the following commits will be added to
For reference, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I'm not knowledgeable enough to critique the yaml
and toml
files but I assume that you've done the right thing there. The md
files look good. For the py
files, my only critique is that taking repr
out of some f-strings means that they'll print the likes of the name is MyFile
rather than the name is 'MyFile'
. If you intended that change then that's fine. If not, perhaps revert that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the commits here should be added to the .git-blame-ignore-revs
.
(Thank you for providing the .git-blame-ignore-revs
in #50 btw)
EDIT: I just noticed your comment above saying you will add to .git-blame-ignore-revs
later. It could just be done here in this PR though right?
The CI stuff like ruff and pre-commit is going over my head but I appreciate having it and know it will make life better. Not sure how I would even test it though, so I'm not including that in my review. Happy to merge and see what happens. Thanks for adding that 😄
Since we will likely integrate with |
Why not do an ordinary merge? This is the problem with rebasing, that it changes the history and therefore the commit hashes Hmm on second thought maybe merge commits themselves can show up in the git blame... idk |
In the context of Slicer-based project like this one, this allows to keep the history simple and easy to comprehend. It also enable us to use the computed revision (monotonously increasing revision number) to compare version. References: |
Haha, okay fine. I always dislike when commit hashes change on me. But I can understand from the references you provided why it can be desirable to have linear history |
Ditto. To clarify, commit already integrated in the |
Add "pre-commit" GitHub Actions workflow with standard pre-commit hooks1 as well as ruff2, prettier3 and schema validations4 for GitHub workflow files.
Footnotes
https://github.com/pre-commit/pre-commit-hooks ↩
https://beta.ruff.rs/docs/ ↩
https://prettier.io/docs/en/ ↩
https://github.com/python-jsonschema/check-jsonschema ↩