Ruff check#428
Closed
mabruzzo wants to merge 2 commits into
Closed
Conversation
Collaborator
Author
|
The changes introduced here actually got merged in with the cholla_utils PR. Consequently, I'm going to close it |
This file contains hidden or 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
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.
EDIT: the contributions of this PR are now a part of #434. We can close this PR if we want
Overview
This adds the ruff linter to pre-commit.
Motivation
In more detail, the ruff linter performs similar actions to
clang-tidy, but applies to python.Important
Ruff also provides a code-formatter (e.g. like clang-format for python). This PR does NOT enable the formatter. While I think it would be useful to enable the code formatter, that's beyond the scope of this PR (it would produce a bunch of churn in the existing scripts and it provides less value)
It's useful to draw comparisons to
clang-tidy:myvar(or I defined it asmy_var). Python won't identify the error unless we run the code andcondhas a value ofFalse. The ruff linter provides a lot of additional value2 by identifying these cases for us!The second bullet point covers the majority of python bugs that I personally produce (and that I see). And for that reason I think the linter is extremely useful (I found it useful while writing #427)
Details
Currently the ruff linter identifies lots of "problems" with our existing scripts. While many of these problems are about coding-style, some of them are real issues. To avoid making this a large PR, I configured the ruff linter to exclude all existing files and only apply to newly introduced files. But, we can fix that going forward.
As a proof of concept, I did address the "problems" in 2 existing files -- pertaining to unused import-statements
Footnotes
I suspect that this has a lot to do with the kind of logic within Cholla. I suspect that in more-general codebases (not just a numerical simulation code) it's probably a more useful tool. ↩
If this logic were in C/C++, clang-tidy could probably also identify this case for us. But this isn't as useful since any modern C/C++ compiler should also identify the error. ↩