Project

General

Profile

Actions

Idea #19821

closed

Write docstrings for arvados.collection

Added by Brett Smith over 1 year ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Documentation
Story points:
1.0
Release relationship:
Auto

Description

Ensure all public classes, methods, and functions in the arvados.collection module have docstrings in our style.


Subtasks 1 (0 open1 closed)

Task #21010: Review 19821-collection-docstringsResolvedBrett Smith10/11/2023Actions

Related issues

Related to Arvados Epics - Idea #18800: Update Python SDK documentationIn Progress11/01/202203/31/2024Actions
Actions #1

Updated by Brett Smith over 1 year ago

  • Related to Idea #18800: Update Python SDK documentation added
Actions #2

Updated by Peter Amstutz over 1 year ago

  • Target version set to Future
Actions #3

Updated by Peter Amstutz over 1 year ago

  • Assigned To set to Brett Smith
Actions #4

Updated by Brett Smith over 1 year ago

  • Story points set to 1.0
  • Description updated (diff)
Actions #5

Updated by Brett Smith about 1 year ago

  • Target version changed from Future to To be scheduled
Actions #6

Updated by Peter Amstutz 7 months ago

  • Target version changed from To be scheduled to Development 2023-10-11 sprint
Actions #7

Updated by Brett Smith 6 months ago

  • Status changed from New to In Progress
Actions #8

Updated by Brett Smith 6 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.

Actions #9

Updated by Peter Amstutz 6 months ago

  • Target version changed from Development 2023-10-11 sprint to Development 2023-10-25 sprint
Actions #10

Updated by Brett Smith 6 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
Actions #11

Updated by Lucas Di Pentima 6 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 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)
Actions #12

Updated by Brett Smith 6 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 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

Actions #13

Updated by Lucas Di Pentima 6 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!

Actions #14

Updated by Brett Smith 6 months ago

  • Status changed from In Progress to Resolved
Actions #15

Updated by Brett Smith 5 months ago

  • Release set to 67
Actions

Also available in: Atom PDF