-
Notifications
You must be signed in to change notification settings - Fork 9
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
gh-231: add ruff
linting to pre-commit
#232
Conversation
@@ -2,3 +2,4 @@ | |||
b42067f6776dcd763827000d585a88e071b78af3 | |||
# applied prettier - https://github.com/glass-dev/glass/pull/227 | |||
f9bac62f497a7288aa71fd4dbd948945c37f854e | |||
# applied ruff-linting - https://github.com/glass-dev/glass/pull/232 |
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.
@Saransh-cpp when this is merged can you add the commit hash underneath?
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.
🫡
"ANN001", # TODO: missing-type-function-argument | ||
"ANN002", # TODO: missing-type-args | ||
"ANN003", # TODO: missing-type-kwargs | ||
"ANN201", # TODO: missing-return-type-undocumented-public-function | ||
"COM812", # missing-trailing-comma (ruff-format recommended) | ||
"D203", # one-blank-line-before-class | ||
"D212", # blank-line-before-class | ||
"ERA001", # TODO: commented-out-code | ||
"ISC001", # single-line-implicit-string-concatenation (ruff-format recommended) | ||
"NPY002", # TODO: numpy-legacy-random | ||
"NPY201", # TODO: numpy2-deprecation | ||
"RUF003", # ambiguous-unicode-character-comment |
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.
All the TODO
can gradually be fixed, not doing now for the sake of time
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.
Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk>
Okay, this is ready, always fiddly to add this kind of thing! Please review carefully as there are a fair few changes. They are generally trivial. I have self-reviewed and everything looks fine, I think. Have raised #248, #249 to follow up. This should probably me merged ASAP as other PRs will conflict with this potentially - and a fair bit of effort to fix. |
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.
Amazing piece of work! Had a quick look and other than the headings I think everything is a matter of taste.
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.
Taking over 🫡
This should be ready to review again! |
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.
Amazing work, thanks! Looks good to me. I'll merge after the mean
/mode
confusion is cleared.
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.
Excellent work @paddyroddy and @Saransh-cpp!
Thanks for taking over whilst I was away @Saransh-cpp! |
Add
ruff
linting and turn on as many rules as possible. However, I will turn off many and not spend too much time fixing them. In a perfect world, we will try and fix as many of these as possible - as they will generally help with maintenance in the long term.Closes: #231
Refs: #170