Project

Profile

Help

Task #2347

closed

Update style guide with Google docstring style docblocks

Added by amacdona@redhat.com over 5 years ago. Updated over 2 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Category:
-
Sprint/Milestone:
Start date:
Due date:
% Done:

100%

Estimated time:
Platform Release:
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Sprint:
Sprint 21
Quarter:

Description

Reasoning: Our docstring formatting could be easier to visually parse.

:param repo_name: The name of the repo
:type  repo_name: basestring

This obviously has a very low information density. In Google docstring style this would be:

    Params:
        repo (basestring): Name of a repository

Format can't fix awkward, and this is still not a super helpful comment,
but at least it takes up less space but contains the same information.
The lists will be shorter and easier on the eyes.

It would also be easier to scan if the first part of the line was a variable
name instead of `:param:`.

Take a look at a comparison of larger docstrings from the wild:

def _process_repos(repo_objs, details, importers, distributors):
    """
    Serialize repository objects and add related importers and distributors if requested.

    Apply standard processing to a collection of repositories being returned to a client. Adds 
    the object link and optionally adds related importers and distributors.

    :param repo_objs:    collection of repository objects
    :type  repo_objs:    list or tuple of pulp.server.db.model.Repository objects
    :param details:      if True, include importers and distributors, overrides other values
    :type  details:      bool
    :param importers:    if True, adds related importers under the attribute "importers"
    :type  importers:    bool
    :param distributors: if True, adds related distributors udef _process_repos(repo_objs, details, importers, distributors):
    :type  distributors: bool
    """

In Google docstring style this would be:

def _process_repos(repo_objs, details, importers, distributors):
    """
    Serialize repository objects and add related importers and distributors if requested.

    Apply standard processing to a collection of repositories being returned to a client. Adds 
    the object link and optionally adds related importers and distributors.

     Args:
          repo_objs (list or tuple of Repository): collection of repository objects
          details (bool): if True, include importers and distributors, overrides other values
          importers (bool): if True, adds related importers under the attribute"importers".
          distributors (bool): if True, adds related distributors under the attribute "distributors" 

    Returns:
          List of serialized repositories with importer and distributor data optionally added
    """       

We can use Napolean[0] as a Sphinx plugin to enable support for Sphinx to consume and present Google style docstrings.

[0]: http://www.sphinx-doc.org/en/stable/ext/napoleon.html

Actions #1

Updated by amacdona@redhat.com over 5 years ago

  • Description updated (diff)
Actions #2

Updated by amacdona@redhat.com over 5 years ago

  • Description updated (diff)
Actions #4

Updated by semyers over 5 years ago

mhrivnak was expressing an aesthetic preference for more tabular layouts. That's still possible with napoleon, so I thought I'd present it as another potential option:

    """ 
    Serialize repository objects and add related importers and distributors if requested.

    Apply standard processing to a collection of repositories being returned to a client. Adds 
    the object link and optionally adds related importers and distributors.

     Args:
          repo_objs    (list): collection of repository objects
          details      (bool): if True, include importers and distributors, overrides other values
          importers    (bool): if True, adds related importers under the attribute"importers".
          distributors (bool): if True, adds related distributors under the attribute "distributors" 

    Returns:
          list: serialized repositories with importer and distributor data optionally added
    """    

I took some liberties with "repo_objs", because it ruins everything in both :sphinx: syntax and napoleon (sphinx doesn't really understand compound types like Repository[], the type of that arg should be list, or "Iterable" [0] )

I'm not a fan of this "tabular" style, but others might be. Thoughts?

[0]: https://docs.python.org/3/library/collections.abc.html#collections.abc.Iterable

Actions #5

Updated by bmbouter over 5 years ago

+1 to this story. I want to edit it to replace s/Napolean/Google style docstrings/ to distinguish the style change versus the Sphinx plugin which will parse Google style docstrings. Am I OK to edit this some?

Actions #6

Updated by bmbouter over 5 years ago

Also I prefer the non-tabbed version. It is a bit more difficult to read, but I like being efficient with the whitespace to have fewer overall lines.

Actions #7

Updated by semyers over 5 years ago

There is no plugin to install. napoleon is bundled with sphinx as of Sphinx 1.3, and we're currently getting 1.4.8 from pypi.

Actions #8

Updated by semyers over 5 years ago

bmbouter wrote:

+1 to this story. I want to edit it to replace s/Napolean/Google style docstrings/ to distinguish the style change versus the Sphinx plugin which will parse Google style docstrings. Am I OK to edit this some?

Yeah, I think that's a great idea.

Actions #9

Updated by bmbouter over 5 years ago

  • Subject changed from Update style guide with Napoleon style docblocks to Update style guide with Google docstring style docblocks
  • Description updated (diff)

I rewrote the story some to differentiate the style choice from the technology.

Actions #10

Updated by jortel@redhat.com over 5 years ago

+1 to the story. I'm also not a fan of the tabular style.

Actions #11

Updated by mhrivnak over 5 years ago

  • Description updated (diff)
Actions #12

Updated by mhrivnak over 5 years ago

I'm happy accepting this as groomed once we answer just one more question: what will the transition be like?

Options:

  • Use the new style when writing new doc blocks, and optionally when updating old ones. Otherwise leave old ones as-is.
  • Spend (a lot of) time converting all doc blocks to the new style.
  • Find or make a tool to automatically do much of the conversion.

Preferences? While I'm not excited about the idea of manually converting everything over, I also wonder if having a mix of the two styles would become enough of an annoyance to be distracting. I think I could live with the mix for quite a while, but would like other opinions.

I think we should have a plan one way or the other before accepting the stylistic change.

Actions #13

Updated by mhrivnak over 5 years ago

As an aside, I think (and hope) it's an exaggeration to use words like "drowning" and "assault" is describing the current style. This issue reads more like an editorial than I think a task normally should.

I'd generally like to see a more balanced comparison of the options when they take the form of a redmine issue, in any situation where we are choosing from multiple options. There are reasonable, rational people who make the choice on each side, so let's take an objective look at why. I understand that this issue was filed as a proposal, so there is a natural element of advocacy for a particular choice. That is fine and productive. But that advocacy, when in the form of an issue to be worked on by an engineer, should be done in a balanced and objective way that considers each option seriously.

If you do feel strongly about something, and you want to write an editorial-style persuasive piece advocating for a particular choice, that's very valuable. But I think it should be separate from the body of a redmine issue. And even when you make a persuasive argument in other venues, consider that the use of hyperbole does not invite or promote the contribution of differing perspectives.

In this case, it's clear that most of us prefer the Google style, and that's great. Nobody has expressed that they dislike it, which is even better. I think we've arrived at a fine path forward, but I wanted to point this out for the benefit of future proposals.

Actions #14

Updated by semyers over 5 years ago

mhrivnak wrote:

I'm happy accepting this as groomed once we answer just one more question: what will the transition be like?

Options:

  • Use the new style when writing new doc blocks, and optionally when updating old ones. Otherwise leave old ones as-is.
  • Spend (a lot of) time converting all doc blocks to the new style.
  • Find or make a tool to automatically do much of the conversion.

Preferences? While I'm not excited about the idea of manually converting everything over, I also wonder if having a mix of the two styles would become enough of an annoyance to be distracting. I think I could live with the mix for quite a while, but would like other opinions.

I think we should have a plan one way or the other before accepting the stylistic change.

I converted many existing docstrings, though not to any accepted style, in https://github.com/pulp/pulp/pull/2789. It wasn't that much work, but was constrained to the ReST-style docstrings in the "pulp.app" namespace. So this is my preference:

  • Spend (a lot of) time converting all doc blocks to the new style in new code.

As (maybe) expected, in order to not spend a lot of time converting all the existing docs, I took this approach:

  • Find or make a tool to automatically do much of the conversion.

The gist is some (vim-style) regexen: %s/\(\s*\):param\s\+\(.*\):\s\+\(.*\)\n\s*:type\s\+.\+:\s\+\(.*\)/\1 \2 (\4): \3 to catch all the typed params, and s/\(\s*\):param\s\+\(.*\):\s\+\(.*\)/\1 \2: \3 for untyped.

This needs to be modified if other (typed or untyped) params exist, such as :ivar:/:vartype: combos. After running the regex, the only work left is pasting in the section headers (e.g. "Args:", etc.)

mhrivnak wrote:

As an aside, <snip>...
If you do feel strongly about something, and you want to write an editorial-style persuasive piece advocating for a particular choice, that's very valuable. But I think it should be separate from the body of a redmine issue. And even when you make a persuasive argument in other venues, consider that the use of hyperbole does not invite or promote the contribution of differing perspectives.

This is a valuable point, and maybe worthy of discussion on pulp-dev, and/or adapted to fit into our current "writing a story" docs[0]. I'm not sure how to track that, but at the very least I don't want this to be lost in the guts of a redmine issue.

[0]: http://docs.pulpproject.org/en/2.12/nightly/dev-guide/contributing/features.html?highlight=story#writing-a-story

Actions #15

Updated by bmbouter over 5 years ago

Actions #16

Updated by bmbouter over 5 years ago

  • Tags Pulp 3 added

I'm tagging this as Pulp3. I don't think we're considering this change for 2.y.

The Napoleon config is already enabled[0] on 3.0 so I checked that item.

[0]: https://github.com/pulp/pulp/pull/2789/files#diff-85987f48f1258d9ee486e3191495582dR32

Actions #17

Updated by bmbouter over 5 years ago

Actions #18

Updated by bmbouter over 5 years ago

Actions #19

Updated by bmbouter over 5 years ago

  • Sprint Candidate changed from No to Yes

Of the options presented I'm proposing that we should have new dosctrings use the new style and have code ported from Pulp 2.y updated when it is moved into its new home in Pulp3. To do this, we can:

  • Declare this style change in the 3.0-dev docs style guide[0]
  • Update the 3.0 development guide[1] to identify the requirement to port docstrings and give an example of how to use this regex to ease the pain.

I've updated the checklist items to reflect this new plan and marked this as a sprint candidate.

[0]: https://github.com/pulp/pulp/blob/3.0-dev/docs/contributing/style_guide.rst
[1]: https://github.com/pulp/pulp/tree/3.0-dev/docs/contributing/3.0-development

Actions #20

Updated by semyers over 5 years ago

  • Groomed changed from No to Yes
Actions #21

Updated by mhrivnak over 5 years ago

  • Sprint/Milestone set to 30
Actions #22

Updated by mhrivnak over 5 years ago

  • Sprint/Milestone changed from 30 to 36
Actions #23

Updated by semyers over 5 years ago

We've identified this as a great opportunity to use a PUP for documenting the pros and cons of using Napoleon for formal acceptance by the team before adding its use to our dev docs.

Actions #24

Updated by amacdona@redhat.com over 5 years ago

  • Description updated (diff)
Actions #26

Updated by amacdona@redhat.com over 5 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to amacdona@redhat.com
Actions #27

Updated by amacdona@redhat.com over 5 years ago

This task is now up for consideration as a PUP. Other work is halted until PUP is approved.

Voting is scheduled to be finished on April 10. 2017.

https://github.com/pulp/pups/pull/2

Actions #28

Updated by mhrivnak about 5 years ago

  • Sprint/Milestone changed from 36 to 37
Actions #29

Updated by jortel@redhat.com about 5 years ago

  • Sprint/Milestone changed from 37 to 38
Actions #30

Updated by mhrivnak about 5 years ago

  • Sprint/Milestone changed from 38 to 39
Actions #32

Updated by mhrivnak about 5 years ago

  • Sprint/Milestone changed from 39 to 40

Added by amacdona@redhat.com about 5 years ago

Revision 2c134bba

Create style guide for Pulp 3+

closes #2347

Added by amacdona@redhat.com about 5 years ago

Revision 2c134bba

Create style guide for Pulp 3+

closes #2347

Actions #33

Updated by amacdona@redhat.com about 5 years ago

  • Status changed from ASSIGNED to MODIFIED
  • % Done changed from 0 to 100

Added by amacdona@redhat.com about 5 years ago

Revision e0dbed74

Add note to convert ported docstrings

re #2347

Added by amacdona@redhat.com about 5 years ago

Revision e0dbed74

Add note to convert ported docstrings

re #2347

Actions #35

Updated by bmbouter over 4 years ago

  • Sprint set to Sprint 21
Actions #36

Updated by bmbouter over 4 years ago

  • Sprint/Milestone deleted (40)
Actions #37

Updated by daviddavis about 3 years ago

  • Sprint/Milestone set to 3.0.0
Actions #38

Updated by bmbouter about 3 years ago

  • Tags deleted (Pulp 3)
Actions #39

Updated by bmbouter over 2 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF