Move type annotations from docstrings to signatures wherever possible

openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io

http://openforcefield.org

MIT License

311 stars 90 forks source link

Move type annotations from docstrings to signatures wherever possible #1791

Closed Yoshanuikabundi closed 6 months ago

Yoshanuikabundi commented 9 months ago

I don't think we'll be able to do this in a single PR, but it would be good to consolidate all type information in signatures and type annotations, rather than docstrings. Sphinx is still able to present these types, and having them be actually type checked makes more documentation testable.

mattwthompson commented 9 months ago

Is it worth removing existing : str, optional, default=None from docstrings (in addition to adding relevant annotations)? I originally thought no on the basis that removing something that works is bad, but after a few days I've come around to thinking it's worth it.

Type annotations are significantly more visible to current tooling, probably more true in the future
- This helps surface potential issues and warts ... such as the default value for a string argument being None
Type annotations are more friendly to downstream developers
Documentation builders work just as fine with either (I think? Is one better?)
Duplicate sources of truth are bad (some already exist)

Yoshanuikabundi commented 9 months ago

Yes, I think we should definitely remove any information from docstrings that's duplicated in the signature. In addition to the points you mention, docstrings go out of date really fast - every time I go over the docs of any project I find incorrect docstrings.

Documentation builders work just as fine with either (I think? Is one better?)

Annotations are generally better as they reliably appear in the signature, rather than just in the text - and importantly, are much more testable.

I think any one of these issues is enough to convince me, all of them together makes me very confident that docstrings should not include information duplicated from the signature.

mattwthompson commented 9 months ago

I mostly implemented this yesterday; it felt like sending one's favorite cattle out to pasture, if even for the better.

The only concern I had was how to communicate the default value - I'm not sure how best to do it, and if I can con you into being the reviewer for these changes you'll see there