Closed irm-codebase closed 6 days ago
Attention: Patch coverage is 79.34783%
with 19 lines
in your changes missing coverage. Please review.
Project coverage is 95.33%. Comparing base (
872978d
) to head (00efb50
). Report is 14 commits behind head on main.
pre-commit
breaking is expected. I will fix these issues before merging this PR, but only after confirming these new lints are desired.
If you're happy to fix the issues raised by
precommit.ci
then I'm strongly in favour of this!
Happy is a strong word for linting fixes... but I will do it hahaha.
@irm-codebase don't forget to add yourself to the list in AUTHORS
during this PR
@brynpickering good chuck of files done. Most of these are minor changes. I am not completely re-working the documentation (I think it's better/safer to do that as we go on).
Question though: is it possible to configure pre-commit to only run "D" (pydocstrings) for merges? I think it makes sense to allow people to develop their code with docstring flexiblity, and only enforce it before bringing it into main.
I've read a bit of pre-commit's configuration, and it seems possible, but I am not experienced enough with setting hooks to know how to achieve it.
This additional setting in pr-ci.yml
could do the trick. It should only run pydocstyle on changed files, and ignore all others. I tried testing it, but with no luck. I'm probably missing something.
Advantages: should request updated dostrings as we develop, making this PR a bit easier. Disadvantages: if you modify a test file, you are in for a nasty surprise since you need to add docstrings to all tests.
ruff-pydocstyle:
runs-on: ubuntu-latest
if: startsWith(github.event.pull_request.title, 'Release v')
steps:
- uses: tj-actions/changed-files@v44
- uses: chartboost/ruff-action@v1
with:
args: check --select D
I think I prefer it being there by default. Hopefully shouldn't be too much effort to keep pre-commit / ruff happy with additions once you have the fixes done in this PR. It instills good documenting practice from our development team, which is no bad thing ;)
@irm-codebase while you're here, could you also remove our dependence on Black and move entirely to Ruff (as we do in euro-calliope
)?
@brynpickering corrections implemented!
Sadly the only way to avoid the @abstractmethod
issue is to deactivate linting for those functions.
We could implement the @override
decorator to avoid this, but it's python 3.12 exclusive, and will break our tests for 3.10 / 3.11.
No idea why codeconv does not like this update, there is nothing added to the code :shrug:
edit: it's because of the added ...
in some @abstractmethod
cases. But without it we get linter issues.
edit: it's because of the added ... in some @abstractmethod cases. But without it we get linter issues.
Yeah, I think that's why I had docstrings in them to begin with, to keep codecov happy. I'm not sure there's much to be done about it (or, at least, I can't find an obvious answer online).
Thanks for the help @brynpickering! I went through your changes and I agree to them. Is there anything else left to do for this PR?
607 / #611 mostly happened because inputs and docstrings were not being evaluated by the linter.
I propose to activate additional linting. Docstring linting tends to be annoying, but in our case I think it'll help. It's very likely that this will trigger some hassle when modifying code at the start, but in the long run it will make our lives easier.
Additional linting is not always well received, though. Please let me know what you think!
Summary of changes in this pull request
Reviewer checklist