Fix up docstrings in rbtools/clients/

Review Request #9884 — Created April 21, 2018 and submitted

Information

RBTools
release-1.0.x
15e7b72...

Reviewers

This change fixes up all the docstrings for the various clients to match
our modern style, and to be consistent across the different tools. Whew.

  • Ran unit tests.
  • Ran pydocstyle.
Description From Last Updated

, optional

chipx86chipx86

, optional

chipx86chipx86

"Return"

chipx86chipx86

"command line" is more common these days than "command-line". I only see "command-line" in terms of "command-line interface," but even …

chipx86chipx86

We don't need to say "The 'revisions' argument" here.

chipx86chipx86

Let's use double backticks for the examples.

chipx86chipx86

This will parse wrong, due to the indentation. In other places we do: A dictionary with the following keys: ``base`` …

chipx86chipx86

I don't think we want to reference git here.

chipx86chipx86

We should probably document these. Certainly which are required and which are not. Also, it mentions commit_id but that's not …

chipx86chipx86

:command: for patch

chipx86chipx86

"filenames"

chipx86chipx86

Can you make the patch -pX a literal?

chipx86chipx86

I had to look up how this was actually provided. At first I thought a dictionary, but then I saw …

chipx86chipx86

, optional

chipx86chipx86

, optional

chipx86chipx86

Maybe reference parse_revision_spec here?

chipx86chipx86

Same note as above with the author info.

chipx86chipx86

Let's get rid of "RB" here. Nothing else really references that.

chipx86chipx86

This isn't showing as optional in the function parameters.

chipx86chipx86

These all need , optional

chipx86chipx86

"Return"

chipx86chipx86

This isn't optional in the function parameters.

chipx86chipx86

"Windows"

chipx86chipx86

"ClearCase"

chipx86chipx86

I don't know how this is going to look, but it's probably not going to be a suitable list. Can …

chipx86chipx86

Can you fix the missing space in this comment while here, and change to "ClearCase"?

chipx86chipx86

"Return"

chipx86chipx86

Mind changing this while here to not override _? I feel like this is going to bite us someday. A …

chipx86chipx86

This can be a list as well.

chipx86chipx86

Extra space before the ending paren.

chipx86chipx86

, optional

chipx86chipx86

"Return"

chipx86chipx86

Missing the full module path.

chipx86chipx86

Is this a SHA?

chipx86chipx86

, optional

chipx86chipx86

Can you make the tuple example a literal?

chipx86chipx86

list of unicode?

chipx86chipx86

This isn't listed as optional in the function params.

chipx86chipx86

"Perforce"

chipx86chipx86

"Perforce"

chipx86chipx86

Can you use literals for these?

chipx86chipx86

"Perforce"

chipx86chipx86

"Can you use a literal here?"

chipx86chipx86

Missing a type and , optional

chipx86chipx86

Missing a period.

chipx86chipx86

"Perforce"

chipx86chipx86

, optional

chipx86chipx86

, optional

chipx86chipx86

"Return"

chipx86chipx86

"URL"

chipx86chipx86

Can you use literals for the quoted bits? Also, "URL".

chipx86chipx86

"URLs"

chipx86chipx86

"extraction"

chipx86chipx86

"extraction"

chipx86chipx86

"extraction"

chipx86chipx86

"extraction"

chipx86chipx86

"exclude_patterns" isn't documented.

chipx86chipx86

Missing a name.

chipx86chipx86

These should probably be like: ``//path/to/file`` Description... ``//path/to/dir...``` Description...

chipx86chipx86

This isn't optional in the function params.

chipx86chipx86

"Perforce"

chipx86chipx86

"Return"

chipx86chipx86

This should probably be removed.

chipx86chipx86

"Return"

chipx86chipx86

These aren't optional in the function params.

chipx86chipx86

We shouldn't use "r". Instead, we need to fix the example.

chipx86chipx86

The example diffs should all be indented 4 spaces with a blank line on either side, to turn them into …

chipx86chipx86

:command:

chipx86chipx86

This isn't optional in the function params.

chipx86chipx86

We can nix "RB", or change it to "Review Board."

chipx86chipx86

E501 line too long (80 > 79 characters)

reviewbotreviewbot

We should use :command: here as well.

chipx86chipx86

:py:meth:

chipx86chipx86

rbtools.clients.errors.MergeError

chipx86chipx86

rbtools.clients.errors.PushError

chipx86chipx86

rbtools.clients.errors.AmendError

chipx86chipx86

Missing a "Returns."

chipx86chipx86

, optional

chipx86chipx86

:command: around "cleartool"?

chipx86chipx86

Can we rewrite this? It's not clear. Maybe something like: This will split a version path, pulling out the branch …

chipx86chipx86

Like above, this wording isn't ideal. How about: """Construct an extended path from a file path and version identifier. This …

chipx86chipx86

How about "Construct a revisioned path from a branch path and version identifier."

chipx86chipx86

How about "Return extended paths for all modifications in a changeset."

chipx86chipx86

Can we make this a bit more English-sounding?

chipx86chipx86

Can we use :command: around "cleartool lsX"?

chipx86chipx86

We should use :command: for "cvs diff".

chipx86chipx86

Can we make the refs/heads/ a literal?

chipx86chipx86

:command: for "git config"

chipx86chipx86

:command: for "svn diff".

chipx86chipx86

Let's use :command: around git-svn and git-p4.

chipx86chipx86

:command: here as well.

chipx86chipx86

And on these.

chipx86chipx86

rbtools.clients.errors.AmendError

chipx86chipx86

Let's use :envvar around $EDITOR.

chipx86chipx86

rbtools.clients.errors.MergeError

chipx86chipx86

rbtools.clients.errors.PushError

chipx86chipx86

This should specify that None will be returned if the path could not be found.

chipx86chipx86

Let's use :command: around "p4 info".

chipx86chipx86

:command: around "p4"

chipx86chipx86

:command: around "p4"

chipx86chipx86

:command: around "p4"

chipx86chipx86

Should be "Returns" and then "Raises"

chipx86chipx86

"Returns" and then "Raises"

chipx86chipx86

"Returns" and then "Raises"

chipx86chipx86

"Returns" and then "Raises"

chipx86chipx86

The - should be a _

chipx86chipx86

rbtools.clients.errors.AmendError

chipx86chipx86

Need a double :: in the preceding paragraphs, or this will appear as quoted text (for like e-mails and such).

chipx86chipx86

"Returns" and then "Raises"

chipx86chipx86

If you are running flake8 with the flake8-docstrings package, we should be able to have these both in flake8.ignore.

brenniebrennie
chipx86
  1. Starting off with a review of the base client code, as many of these comments apply to the subclasses as well.

  2. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     
     

    , optional

  3. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     
     

    , optional

  4. rbtools/clients/__init__.py (Diff revision 1)
     
     

    "Return"

  5. rbtools/clients/__init__.py (Diff revision 1)
     
     

    "command line" is more common these days than "command-line". I only see "command-line" in terms of "command-line interface," but even the articles with that use "command line" in other contexts.

  6. rbtools/clients/__init__.py (Diff revision 1)
     
     

    We don't need to say "The 'revisions' argument" here.

  7. rbtools/clients/__init__.py (Diff revision 1)
     
     

    Let's use double backticks for the examples.

  8. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This will parse wrong, due to the indentation. In other places we do:

    A dictionary with the following keys:
    
    ``base`` (:py:class:`unicode`):
        A revision ...
    
    ``tip`` (:py:class:`unicode`):
        ...
    
  9. rbtools/clients/__init__.py (Diff revision 1)
     
     
     

    I don't think we want to reference git here.

  10. rbtools/clients/__init__.py (Diff revision 1)
     
     
     

    We should probably document these. Certainly which are required and which are not.

    Also, it mentions commit_id but that's not included in the return statement.

  11. rbtools/clients/__init__.py (Diff revision 1)
     
     

    :command: for patch

  12. rbtools/clients/__init__.py (Diff revision 1)
     
     

    "filenames"

  13. rbtools/clients/__init__.py (Diff revision 1)
     
     

    Can you make the patch -pX a literal?

  14. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     

    I had to look up how this was actually provided. At first I thought a dictionary, but then I saw that review_request.get_submitter() was passed in. Maybe we should just make this take a dictionary with fullname and email keys and change the one call site?

    1. That sounds nice but not for this change.

  15. rbtools/clients/__init__.py (Diff revision 1)
     
     

    , optional

  16. rbtools/clients/__init__.py (Diff revision 1)
     
     

    , optional

  17. rbtools/clients/__init__.py (Diff revision 1)
     
     
     

    Maybe reference parse_revision_spec here?

  18. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     

    Same note as above with the author info.

  19. rbtools/clients/__init__.py (Diff revision 1)
     
     

    Let's get rid of "RB" here. Nothing else really references that.

  20. rbtools/clients/__init__.py (Diff revision 1)
     
     
     

    This isn't showing as optional in the function parameters.

  21. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These all need , optional

  22. rbtools/clients/bazaar.py (Diff revision 1)
     
     

    "Return"

  23. 
      
chipx86
  1. 
      
  2. rbtools/clients/bazaar.py (Diff revision 1)
     
     
     
     

    This isn't optional in the function parameters.

  3. rbtools/clients/clearcase.py (Diff revision 1)
     
     

    "Windows"

  4. rbtools/clients/clearcase.py (Diff revision 1)
     
     

    "ClearCase"

  5. rbtools/clients/clearcase.py (Diff revision 1)
     
     
     
     

    I don't know how this is going to look, but it's probably not going to be a suitable list. Can you fix this up?

  6. rbtools/clients/clearcase.py (Diff revision 1)
     
     

    Can you fix the missing space in this comment while here, and change to "ClearCase"?

  7. rbtools/clients/cvs.py (Diff revision 1)
     
     

    "Return"

  8. rbtools/clients/git.py (Diff revision 1)
     
     

    Mind changing this while here to not override _? I feel like this is going to bite us someday. A [0] should be sufficient.

  9. rbtools/clients/git.py (Diff revision 1)
     
     

    This can be a list as well.

  10. rbtools/clients/git.py (Diff revision 1)
     
     

    Extra space before the ending paren.

  11. rbtools/clients/git.py (Diff revision 1)
     
     
     
     
     
     
     

    , optional

  12. rbtools/clients/git.py (Diff revision 1)
     
     

    "Return"

  13. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    Missing the full module path.

  14. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    Is this a SHA?

  15. rbtools/clients/mercurial.py (Diff revision 1)
     
     
     
     
     
     
     

    , optional

  16. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    Can you make the tuple example a literal?

  17. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    list of unicode?

  18. rbtools/clients/mercurial.py (Diff revision 1)
     
     
     

    This isn't listed as optional in the function params.

  19. rbtools/clients/perforce.py (Diff revision 1)
     
     
     
     
     
     

    "Perforce"

  20. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "Perforce"

  21. rbtools/clients/perforce.py (Diff revision 1)
     
     

    Can you use literals for these?

  22. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "Perforce"

  23. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "Can you use a literal here?"

  24. rbtools/clients/perforce.py (Diff revision 1)
     
     

    Missing a type and , optional

  25. rbtools/clients/perforce.py (Diff revision 1)
     
     

    Missing a period.

  26. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "Perforce"

  27. rbtools/clients/perforce.py (Diff revision 1)
     
     

    , optional

  28. rbtools/clients/perforce.py (Diff revision 1)
     
     

    , optional

  29. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "Return"

  30. rbtools/clients/perforce.py (Diff revision 1)
     
     
  31. rbtools/clients/perforce.py (Diff revision 1)
     
     
     
     
     

    Can you use literals for the quoted bits?

    Also, "URL".

  32. rbtools/clients/perforce.py (Diff revision 1)
     
     
  33. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "extraction"

  34. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "extraction"

  35. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "extraction"

  36. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "extraction"

  37. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "exclude_patterns" isn't documented.

  38. rbtools/clients/perforce.py (Diff revision 1)
     
     

    Missing a name.

  39. rbtools/clients/perforce.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These should probably be like:

    ``//path/to/file``
        Description...
    
    ``//path/to/dir...```
        Description...
    
  40. rbtools/clients/perforce.py (Diff revision 1)
     
     

    This isn't optional in the function params.

  41. rbtools/clients/perforce.py (Diff revision 1)
     
     

    "Perforce"

  42. rbtools/clients/plastic.py (Diff revision 1)
     
     

    "Return"

  43. rbtools/clients/plastic.py (Diff revision 1)
     
     

    This should probably be removed.

  44. rbtools/clients/svn.py (Diff revision 1)
     
     

    "Return"

  45. rbtools/clients/svn.py (Diff revision 1)
     
     
     
     
     
     
     

    These aren't optional in the function params.

  46. rbtools/clients/svn.py (Diff revision 1)
     
     

    We shouldn't use "r". Instead, we need to fix the example.

  47. rbtools/clients/svn.py (Diff revision 1)
     
     
     
     
     
     
     
     
     

    The example diffs should all be indented 4 spaces with a blank line on either side, to turn them into code blocks. The "\n" will be retained.

  48. rbtools/clients/svn.py (Diff revision 1)
     
     

    :command:

  49. rbtools/clients/svn.py (Diff revision 1)
     
     
     

    This isn't optional in the function params.

  50. rbtools/clients/svn.py (Diff revision 1)
     
     

    We can nix "RB", or change it to "Review Board."

  51. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. A few wording suggestions for ClearCase, a sprinkling of ReST roles (like :command:), and fixes for bad error class paths. Mostly pretty trivial things.

  2. rbtools/clients/__init__.py (Diff revision 3)
     
     

    We should use :command: here as well.

  3. rbtools/clients/__init__.py (Diff revision 3)
     
     

    :py:meth:

  4. rbtools/clients/__init__.py (Diff revision 3)
     
     

    rbtools.clients.errors.MergeError

  5. rbtools/clients/__init__.py (Diff revision 3)
     
     

    rbtools.clients.errors.PushError

  6. rbtools/clients/__init__.py (Diff revision 3)
     
     
     

    Not something that needs to be addressed in this change, but we have different parameter names in different methods for this same type of argument. We should fix that.

  7. rbtools/clients/__init__.py (Diff revision 3)
     
     

    rbtools.clients.errors.AmendError

  8. rbtools/clients/__init__.py (Diff revision 3)
     
     
     
     
     

    Missing a "Returns."

  9. rbtools/clients/bazaar.py (Diff revision 3)
     
     

    , optional

  10. rbtools/clients/clearcase.py (Diff revision 3)
     
     

    :command: around "cleartool"?

  11. rbtools/clients/clearcase.py (Diff revision 3)
     
     
     
     

    Can we rewrite this? It's not clear. Maybe something like:

    This will split a version path, pulling out the branch and version. A special version value of ``CHECKEDOUT`` represents the latest version of a file, similar to ``HEAD`` in many other types of repositories.
    
  12. rbtools/clients/clearcase.py (Diff revision 3)
     
     
     
     
     
     

    Like above, this wording isn't ideal. How about:

    """Construct an extended path from a file path and version identifier.
    
    This will construct a path in the form of ``path@version``. If the
    version is the special value ``CHECKEDOUT``, only the path will be
    returned.
    
    ...
    """
    
  13. rbtools/clients/clearcase.py (Diff revision 3)
     
     

    How about "Construct a revisioned path from a branch path and version identifier."

  14. rbtools/clients/clearcase.py (Diff revision 3)
     
     

    How about "Return extended paths for all modifications in a changeset."

  15. rbtools/clients/clearcase.py (Diff revision 3)
     
     
     

    Can we make this a bit more English-sounding?

  16. rbtools/clients/clearcase.py (Diff revision 3)
     
     

    Can we use :command: around "cleartool lsX"?

  17. rbtools/clients/cvs.py (Diff revision 3)
     
     

    We should use :command: for "cvs diff".

  18. rbtools/clients/git.py (Diff revision 3)
     
     

    Can we make the refs/heads/ a literal?

  19. rbtools/clients/git.py (Diff revision 3)
     
     

    :command: for "git config"

  20. rbtools/clients/git.py (Diff revision 3)
     
     

    :command: for "svn diff".

  21. rbtools/clients/git.py (Diff revision 3)
     
     

    Let's use :command: around git-svn and git-p4.

  22. rbtools/clients/git.py (Diff revision 3)
     
     

    :command: here as well.

  23. rbtools/clients/git.py (Diff revision 3)
     
     

    And on these.

  24. rbtools/clients/git.py (Diff revision 3)
     
     

    rbtools.clients.errors.AmendError

  25. rbtools/clients/git.py (Diff revision 3)
     
     

    Let's use :envvar around $EDITOR.

  26. rbtools/clients/git.py (Diff revision 3)
     
     

    rbtools.clients.errors.MergeError

  27. rbtools/clients/git.py (Diff revision 3)
     
     

    rbtools.clients.errors.PushError

  28. rbtools/clients/git.py (Diff revision 3)
     
     
     

    This should specify that None will be returned if the path could not be found.

  29. rbtools/clients/perforce.py (Diff revision 3)
     
     

    Let's use :command: around "p4 info".

  30. rbtools/clients/perforce.py (Diff revision 3)
     
     

    :command: around "p4"

  31. rbtools/clients/perforce.py (Diff revision 3)
     
     

    :command: around "p4"

  32. rbtools/clients/perforce.py (Diff revision 3)
     
     

    :command: around "p4"

  33. rbtools/clients/perforce.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    Should be "Returns" and then "Raises"

  34. rbtools/clients/perforce.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

    "Returns" and then "Raises"

  35. rbtools/clients/perforce.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

    "Returns" and then "Raises"

  36. rbtools/clients/perforce.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    "Returns" and then "Raises"

  37. rbtools/clients/perforce.py (Diff revision 3)
     
     

    The - should be a _

  38. rbtools/clients/perforce.py (Diff revision 3)
     
     

    rbtools.clients.errors.AmendError

  39. rbtools/clients/svn.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Need a double :: in the preceding paragraphs, or this will appear as quoted text (for like e-mails and such).

  40. rbtools/clients/svn.py (Diff revision 3)
     
     
     
     
     
     
     
     

    "Returns" and then "Raises"

  41. 
      
david
brennie
  1. 
      
  2. setup.cfg (Diff revision 4)
     
     
     

    If you are running flake8 with the flake8-docstrings package, we should be able to have these both in flake8.ignore.

    1. This isn't something I use. If you want to add it, go ahead--I don't have time to install it and test it now.

  3. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (e982800)
Loading...