Add docstring linting

[MoffittAndrew]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?
Originally created to address issue #483, now also address issue #369. Enforcing docstring linting helps keep docstring formatting consistent across the codebase and across the NIU.

What does this PR do?
Removes type hints from all docstrings + slight tweaks to make docstring formatting more consistent across the codebase, falling in line with the numpydoc format.

Now also enforces linting for docstrings, using the pydocstyle ruff ruleset. The ruff config is based on the config from the movement repo, to help keep docstring formatting consistent across different repos. For more info, see this PR, where docstring formatting was implemented in the movement repo.

References

Addresses #483 - Original issue this PR was aiming to close
Addresses #369 - New issue aiming to close
Docstring linting implementation in movement repo

How has this PR been tested?

No code has been modified, so testing is not necessary.

Is this a breaking change?

No.

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[JoeZiminski]

Hi @MoffittAndrew, thanks for this! This is a really useful contribution, its much appreciated. I've looked over the PR and it all looks good, this will definitely be merged. Because it touches a lot of the codebase, it will make sense to merge some other PRs first before this one. But I don't think it will need any further work on your end, I'll let you know when it is ready to be merged. Thanks a lot!

[MoffittAndrew]

Thanks @JoeZiminski! I was also planning on addressing issue #369 (Adding docstring linting), but this will also require lots of manual edits to docstrings. I think it would be best to implement this on the same branch, to avoid having 2 different PRs that make manual changes to docstrings, which could potentially create conflicts and additional unnecessary work. I will continue to commit to this branch, so if anything goes wrong it would be easy to revert back to the current state of the branch. Let me know what you think, as this also means that a single PR is addressing 2 issues, which is typically not great practice.

[JoeZiminski]

Hi @MoffittAndrew, that's a good point. I think in this case because as you say both PRs will be editing the same docstrings extensively, it makes sense to build those changes on top of these. It's not textbook best-practice but often, it's necessary to take such an approach as the alternative is a total nightmare fixing merge conflicts, which can be very error-prone. As such, very happy for you to continue on this PR, thanks!

As a note, it is also possible to post-hoc split this the PR up. Once all additions are committed, you could branch from this branch (A) onto another branch (B). You can then reset branch A back to the commit which removes type hints from the docstring, which would be as in this current PR. Then it can be merged, and you can open a PR for branch B and rebase onto main to drop any shared commits with A. Then B can be merged. This is useful if it really does turn out that it would be much nicer for provenance to have the changes split across two PRs. But for now, I think splitting would be hassle for not much benefit. So please proceed as above. Cheers!

[MoffittAndrew]

Great! I've been working on another branch so far, so I'll merge those changes into this branch and convert to a draft until everything is finished.

Also, thanks for the tip on splitting PRs! I bet that will come in handy in future.

[JoeZiminski]

Hi @MoffittAndrew, thanks a lot for this great PR, it must have been a lot of work! I have finalised it and I think it is good to go. Thanks for the contribution!

To go through the remaining TODOs:

Completed

• Enforce rule D205 (docstrings must be in format of a short 1-line summary, optionally followed by a longer description. Will require lots of re-writing docstrings)
• Enforce rule D401 (Docstring summaries must be written in a non-imperative mood)
• Go through all PLACEHOLDER docstrings and fill them in with an informative description
• Add returns sections
• I just removed docstrings required for all tests as it will be a lot of work to do this and it is less critical. Some of the undocumented functions are very short.

Did not complete:

• Possibly enforce rule D100 (requires all public modules to have a docstring). Decided against this for now in the interest of workload / time.
• (For another PR, separate issue) Enforce the same ruff ruleset as the movement repo. Currently this PR is only addressing docstring formatting, but there are other ruff rules that we can add to make code formatting consistent across the NIU. These can be added as part of a separate PR later.
  great idea
• Enable the name-tests-test hook in pre-commit.yaml - this requires that many of the filenames in the tests/ directory be renamed, but this could cause issues.
  I avoided this for now as we have test_ prefixed files on which pytest automatically runs tests, and supporting files that should not be tested and are not test_ prefixed. However, that hook complained about those files.