Docstring?

XXX is meaningless without some kind of detail. Why is this comment here?

typo: Attrirbutes

commandline -> command line

Creates or updates, and uploads -> Create or update, and upload

Commandline -> Command line

It was already like this, but commandline -> command line.

Can we wrap the arg value in parens?

commandline -> command line

commandline -> command line

Wrapping is a little weird here.

Can we call this supports_commit_history and the method below get_commit_history? Helps to namespace them so we're not having confusion down the road for any reason.

It'd be nice to document what's in here/where it comes from.

Byte string prefix?

Can we simplify the language and say "cannot both be provided" instead of "mutually exclusive?" The latter's going to be easier for non-English speakers.

These would be nice to implement though. In fact, having history enabled by default and not allowing filtering is going to break a whole lot of people. Is this a temporary restriction? What prevents us from being able to filter these?

I don't really like doing this sort of thing. It'd be much nicer to have some proper checks with proper errors than an assertion that a lot of people working with RBTools wrappers and such are going to be confused by if they manage to hit it.

Mind changing the wording here and below to properly say "review request URL" instead of "review URL?"

This was correct before.

Typo: "hvae" -> "have".

Can you call this review_request_id and review_request_url?

Too many "the review" parts in here.

Can we make additional_fields a list of strings?
Wishing we did that with only_links and only_fields in the API calls, but we can start making it right at least in this function.

Two blank lines.

Alphabetical order.

Alphabetical order.

These should be the same import group.

Blank line between these.

These can start on the raise line.

We're repeating tests unnecessarily. How about:

if server_supports_history:
    if review_request:
        with_history = review_request.created_with_history

        if self.options.with_history:
            ...
    else:
        with_history = self._should_post_with_history(...)
else:
    with_history = False


it's not as flat, so.. I dunno, tradeoffs. Throwing it out there.

This must be logging.warning. Can you test this case?

bool

You can do:

only_fields = [
    ...
] + additional_fields

These can be combined into one statement.

Are we checking for emptiness, or None? If the former, we don't even need the check, as it'll be handled for the for loop.

No trailing comma on the last argument. ) should be up against the last value.

Let's set to None in an else.

Use +=. It's faster.

This should mention the conditions for NotImplementedError and the formats (or references to something) for the revisions and results.

Let's use constants for these, to help with readability and consistency.

We don't have these for the others, unfortunately, but can you add a #: ... doc comment for this one?

Should _FIELD_SEP be a byte string? We split a byte string using _NUL and then try to further split using this.

This wasn't updated to be get_commit_history.
Make sure the latest iteration of changes has been fully tested.

rbtools.clients.errors.SCMError.

Need to add with_parent_diff here.

These seem like artificial limitations that should probably not exist

The long command-line arg is just --include

The long command-line arg is just --exclude

These error messages seem highly internal. Is this really what we want to show to a user? (When might this happen?)

Grammar here is weird. How about "One of "diff_history" or "squashed_diff" must be provided to "Post.post_request()."

Ship It!