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.

[MoffittAndrew]

Still TODO:

• Go through all PLACEHOLDER docstrings and fill them in with an informative description
• Possibly enforce rule D100 (requires all public modules to have a docstring)
• 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)
• 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
• (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.

[Oussemagu]

Can I add something here ? or the issue has mostly been solved ?

[adamltyson]

Hi @Oussemagu, as this is @MoffittAndrew's PR, it's up to them. However, it may be easier to look for another issue that could be solved independently of this PR.

[MoffittAndrew]

Hi @Oussemagu, if you would be willing to help out with filling in some of the PLACEHOLDER docstrings, that would be really helpful! Right now I'm working on the files in datashuttle/configs/ and datashuttle/utils/, but I haven't yet gone through the datashuttle/tui/ folder or tests/. Just let me know which one you'll start working on so that I know not to go through that one.

I think the easiest way to split this work is if you create a branch off of my remove_docstring_typehints branch, and then merge your changes back to my branch. If you have any other suggestions on how that could work, let me know! Also, I'd recommend getting the ruff vscode extension if you haven't already, as this will highlight dosctrings that don't comply with the ruff ruleset specified in pyproject.toml. Thanks!

[JoeZiminski]

Hi @MoffittAndrew thanks a lot for this PR, it is very valuable. Do you think you will have time to finish this PR (don't worry about the docstrings for tests)? I'm happy to first fix the merge conflicts. No worries if not however, I'm happy to finalise it and merge. Thanks for all your work on this!