Project

Profile

Help

Task #2347

Update style guide with Google docstring style docblocks

Added by amacdona@redhat.com almost 3 years ago. Updated 6 months ago.

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

100%

Platform Release:
Blocks Release:
Backwards Incompatible:
No
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:
Sprint 21

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 Napolean0 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


Checklist

Associated revisions

Revision 2c134bba View on GitHub
Added by amacdona@redhat.com over 2 years ago

Create style guide for Pulp 3+

closes #2347

Revision 2c134bba View on GitHub
Added by amacdona@redhat.com over 2 years ago

Create style guide for Pulp 3+

closes #2347

Revision 2c134bba View on GitHub
Added by amacdona@redhat.com over 2 years ago

Create style guide for Pulp 3+

closes #2347

Revision e0dbed74 View on GitHub
Added by amacdona@redhat.com over 2 years ago

Add note to convert ported docstrings

re #2347

Revision e0dbed74 View on GitHub
Added by amacdona@redhat.com over 2 years ago

Add note to convert ported docstrings

re #2347

Revision e0dbed74 View on GitHub
Added by amacdona@redhat.com over 2 years ago

Add note to convert ported docstrings

re #2347

History

#1 Updated by amacdona@redhat.com almost 3 years ago

  • Description updated (diff)

#2 Updated by amacdona@redhat.com almost 3 years ago

  • Description updated (diff)

#4 Updated by semyers almost 3 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

#5 Updated by bmbouter almost 3 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?

#6 Updated by bmbouter almost 3 years ago

  • Checklist item update the style guide in pulp dev docs added
  • Checklist item enable the plugin in the sphinx config of platform added
  • Checklist item update the docs builders to install the plugin added

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.

#7 Updated by semyers almost 3 years ago

  • Checklist item deleted (update the docs builders to install the plugin)

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.

#8 Updated by semyers almost 3 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.

#9 Updated by bmbouter almost 3 years ago

  • Checklist item changed from enable the plugin in the sphinx config of platform to enable the Napolean in the sphinx config of platform
  • 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.

#10 Updated by jortel@redhat.com almost 3 years ago

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

#11 Updated by mhrivnak almost 3 years ago

  • Description updated (diff)

#12 Updated by mhrivnak almost 3 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.

#13 Updated by mhrivnak almost 3 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.

#14 Updated by semyers almost 3 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" docs0. 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

#15 Updated by bmbouter almost 3 years ago

  • Checklist item enable the Napolean in the sphinx config of platform set to Done

#16 Updated by bmbouter almost 3 years ago

  • Checklist item changed from enable the Napolean in the sphinx config of platform to enable the Napoleon in the sphinx config of platform
  • 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 enabled0 on 3.0 so I checked that item.

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

#17 Updated by bmbouter almost 3 years ago

  • Checklist item update the style guide in pulp dev docs set to Done

#18 Updated by bmbouter almost 3 years ago

  • Checklist item update the style guide in pulp dev docs set to Not done

#19 Updated by bmbouter almost 3 years ago

  • Checklist item changed from update the style guide in pulp dev docs to update the style guide in docs/contributing/style_guide.rst
  • Checklist item add the requirement that docstrings be converted to docs/contributing/3.0-development added
  • Checklist item add a full vim-regex helper example to docs/contributing/3.0-development added
  • 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 guide0
  • Update the 3.0 development guide1 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

#20 Updated by semyers almost 3 years ago

  • Groomed changed from No to Yes

#21 Updated by mhrivnak almost 3 years ago

  • Sprint/Milestone set to 30

#22 Updated by mhrivnak over 2 years ago

  • Sprint/Milestone changed from 30 to 36

#23 Updated by semyers over 2 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.

#24 Updated by amacdona@redhat.com over 2 years ago

  • Description updated (diff)

#26 Updated by amacdona@redhat.com over 2 years ago

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

#27 Updated by amacdona@redhat.com over 2 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

#28 Updated by mhrivnak over 2 years ago

  • Sprint/Milestone changed from 36 to 37

#29 Updated by jortel@redhat.com over 2 years ago

  • Sprint/Milestone changed from 37 to 38

#30 Updated by mhrivnak over 2 years ago

  • Sprint/Milestone changed from 38 to 39

#32 Updated by mhrivnak over 2 years ago

  • Sprint/Milestone changed from 39 to 40

#33 Updated by amacdona@redhat.com over 2 years ago

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

#35 Updated by bmbouter over 1 year ago

  • Sprint set to Sprint 21

#36 Updated by bmbouter over 1 year ago

  • Sprint/Milestone deleted (40)

#37 Updated by daviddavis 6 months ago

  • Sprint/Milestone set to 3.0

#38 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3)

Please register to edit this issue

Also available in: Atom PDF