-
Notifications
You must be signed in to change notification settings - Fork 15
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
Replace black
with ruff
and add pre-commit
#110
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM!
Thank you @davidwilby, this does look quite useful. I checked the error message of the CI and we now get many failing docstring checks. Can you 1) address the simpler syntax ones 2) silence more complex issues like missing public function docstrings, so that tests pass? It looks like we can ignore error messages in pyproject.toml with:
|
@tom-andersson I've added a number of rules to ignore and performed the remaining linting of docstrings (the code itself was completely compliant from using The remaining linting failures are for missing docstrings on public methods/functions:
I've had a go at adding a few missing docstrings in 678602a but other than that I don't know enough to write them. One option (which I've used in a couple of places) is to add a Could you add some docstrings or advise on those that are remaining when you get the chance? |
Thanks @davidwilby and sorry for the delay. It's helpful to see where docstrings are missing, so let's keep that behaviour in the linter. I'll add a commit to this PR with the missing docstrings, aiming for this week or the next. |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
An update to the docstrings that need attention:
|
Okay, I think there has to be some statute of limitations on pull requests 😄 so I've opted to ignore these missing docstrings (except one I could actually fill in) and I'll open an issue unless you object @tom-andersson - then we can move on with our lives and merge this after nearly 6 months. |
Hey @tom-andersson et al - just a maintenance-related PR for your perusal, feel free to reject if this isn't your thing.
What this PR does
black
withruff
as the linter of choice, partially because of extra features such as linting docstrings in google-style (which black may do as well, but I haven't been able to find it)pre-commit
and a pre-commit config to run the ruff linter and formatter on a pre-commit hook.CONTRIBUTING.md
detailing the use of these tools.What this PR doesn't do
At the moment the CI check fails since
ruff
is checking all the docstrings, we may be able to soften the docstring rules (configured inpyproject.toml
) to something we agree with (e.g. I don't necessarily think that every module needs a docstring)FYI @nilsleh