Idea #19821
closedWrite docstrings for arvados.collection
Added by Brett Smith over 1 year ago. Updated 9 months ago.
Description
Ensure all public classes, methods, and functions in the arvados.collection
module have docstrings in our style.
Related issues
Updated by Brett Smith over 1 year ago
- Related to Idea #18800: Update Python SDK documentation added
Updated by Brett Smith over 1 year ago
- Story points set to 1.0
- Description updated (diff)
Updated by Brett Smith over 1 year ago
- Target version changed from Future to To be scheduled
Updated by Peter Amstutz 11 months ago
- Target version changed from To be scheduled to Development 2023-10-11 sprint
Updated by Brett Smith 11 months ago
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.
Updated by Peter Amstutz 11 months ago
- Target version changed from Development 2023-10-11 sprint to Development 2023-10-25 sprint
Updated by Brett Smith 11 months ago
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
Updated by Lucas Di Pentima 11 months ago
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
andSubcollection
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 theCollection
type linked, butRichCollectionBase.copy()
has its "source_collection" Collection type not linked. - The code example from
CollectionWriter.open()
is not rendering correctly (i.e.: using monospaced font)
Updated by Brett Smith 11 months ago
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
andSubcollection
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 theCollection
type linked, butRichCollectionBase.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
Updated by Lucas Di Pentima 11 months ago
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!
Updated by Brett Smith 11 months ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|266c340d47ed162c44e0e3d321619734d4500109.