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

, optional

, optional

"Return"

"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.

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

Let's use double backticks for the examples.

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`):
    ...

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

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.

:command: for patch

"filenames"

Can you make the patch -pX a literal?

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?

, optional

, optional

Maybe reference parse_revision_spec here?

Same note as above with the author info.

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

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

These all need , optional

"Return"

This isn't optional in the function parameters.

"Windows"

"ClearCase"

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?

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

"Return"

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

This can be a list as well.

Extra space before the ending paren.

, optional

"Return"

Missing the full module path.

Is this a SHA?

, optional

Can you make the tuple example a literal?

list of unicode?

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

"Perforce"

"Perforce"

Can you use literals for these?

"Perforce"

"Can you use a literal here?"

Missing a type and , optional

Missing a period.

"Perforce"

, optional

, optional

"Return"

"URL"

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

"URLs"

"extraction"

"extraction"

"extraction"

"extraction"

"exclude_patterns" isn't documented.

Missing a name.

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

``//path/to/dir...```
    Description...

This isn't optional in the function params.

"Perforce"

"Return"

This should probably be removed.

"Return"

These aren't optional in the function params.

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

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.

:command:

This isn't optional in the function params.

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

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

We should use :command: here as well.

:py:meth:

rbtools.clients.errors.MergeError

rbtools.clients.errors.PushError

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.

rbtools.clients.errors.AmendError

Missing a "Returns."

, optional

:command: around "cleartool"?

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.

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.

...
"""

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

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

Can we make this a bit more English-sounding?

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

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

Can we make the refs/heads/ a literal?

:command: for "git config"

:command: for "svn diff".

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

:command: here as well.

And on these.

rbtools.clients.errors.AmendError

Let's use :envvar around $EDITOR.

rbtools.clients.errors.MergeError

rbtools.clients.errors.PushError

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

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

:command: around "p4"

:command: around "p4"

:command: around "p4"

Should be "Returns" and then "Raises"

"Returns" and then "Raises"

"Returns" and then "Raises"

"Returns" and then "Raises"

The - should be a _

rbtools.clients.errors.AmendError

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

"Returns" and then "Raises"

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

Ship It!