Arvados

Discussed at standup that we are okay with having type annotations in the method signatures as a documentation aid. These aren't checked and we're not trying to run a type checker on them at this time, it just makes the documentation easier to follow.

19821-collection-docstrings @ ade2c46d4922fff269f4708a0f5a623a3d10bc6c - developer-run-tests: #3858

• All agreed upon points are implemented / addressed.
  • I think in spirit, but a couple of notes about the implementation. Both pydoc and pdoc have a concept of docstring inheritance: if a method in a class doesn't have a docstring, it'll try to get one from a superclass' definition. In this way, I think all methods have docstrings that'll render in any viewing tool, even when not every override has one. I didn't bother documenting methods on deprecated classes.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • N/A
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • Yes, see above. Also reviewed the pdoc rendering manually.
• Documentation has been updated.
  • N/A
• Behaves appropriately at the intended scale (describe intended scale).
  • N/A
• Considered backwards and forwards compatibility issues between client and server.
  • For what it's worth, note that the type annotations in the code are written in a way that's compatible with Python 3.6, while the types in the docstrings are written in a more modern style that was added in Python 3.10 and generally seems preferred for reading. They should be equivalent.
• Follows our coding standards and GUI style guidelines.
  • Yes

Some comments:

• As a suggestion for the next doc story, I think it would be useful to move the deprecated code on a different commit from the new docstring writing. This would make the diff a lot easier to read.
• On the Collection and Subcollection classes (and some others), there're some public members (like num_retries, lock, events) that get mentioned on the rendered doc page but don't have any explanation. Not sure if they should have some note attached.
• In the "Arguments" sections, sometimes references to other classes are linked, and sometimes they aren't. Would be good to have consistency on this. Example: CollectionReader's constructor "parent" has the Collection type linked, but RichCollectionBase.copy() has its "source_collection" Collection type not linked.
• The code example from CollectionWriter.open() is not rendering correctly (i.e.: using monospaced font)

Lucas Di Pentima wrote in #note-11:

• As a suggestion for the next doc story, I think it would be useful to move the deprecated code on a different commit from the new docstring writing. This would make the diff a lot easier to read.

On a personal level, I agree completely. I kind of mixed the two while I was still getting the lay of the land and understanding how things got rendered, but at that point it was hard to unwind after I realized how the diff looked. Sorry.

On a team level, the Arvados Git history is more than kind of a mess. It feels a little weird to get this comment when we regularly have commits with messages like "review comments" that address four unrelated issues.

• On the Collection and Subcollection classes (and some others), there're some public members (like num_retries, lock, events) that get mentioned on the rendered doc page but don't have any explanation. Not sure if they should have some note attached.

It's a fair point and I'm open to discussing it but right now I'm leaning towards no. In general we don't expect users to modify those attributes so I'd rather leave them undocumented. We can always revisit later if we change our mind.

• In the "Arguments" sections, sometimes references to other classes are linked, and sometimes they aren't. Would be good to have consistency on this. Example: CollectionReader's constructor "parent" has the Collection type linked, but RichCollectionBase.copy() has its "source_collection" Collection type not linked.

Fixed.

• The code example from CollectionWriter.open() is not rendering correctly (i.e.: using monospaced font)

I fixed this too, but note CollectionWriter is deprecated so I don't think it's worth spending much time making its documentation consistent with our current standards. Same goes for its subclasses.

Now at e9ed1eb8ec39f40d859489cc2b66687328b3adbc - developer-run-tests: #3861

Brett Smith wrote in #note-12:

On a team level, the Arvados Git history is more than kind of a mess. It feels a little weird to get this comment when we regularly have commits with messages like "review comments" that address four unrelated issues.

Yeah, I think it's useful to remind the reviewees that making the reviewer's life easier is good for the overall process, and I always try to mention that whenever I think there's opportunity for improvement.

Just one observation:

• There's one typo at sdk/python/arvados/collection.py:697

The rest LGTM, thanks!