Fix up docstrings in rbtools/clients/

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

david
RBTools
release-1.0.x
15e7b72...
rbtools

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.
  • 0
  • 0
  • 107
  • 3
  • 110
Description From Last Updated
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...