Can you expand this docstring for arguments?

HttpRequest

Double backticks here.

HttpRequest

Instead of this method, which must be opted into, I think I'd feel more comfortable if we inversed how these querysets were built.
get_queryset() should build the initial queryset that handles diffset__revision (which is consistent between the implementations) and the commit filtering.
It would then call a method defined by the class/subclass for filtering the review request association, like get_diffset_query. That handles the diffset__history__review_request= here, and the diffset__review_request_draft= in DraftFileDiffResource.
This simplifies implementations and keeps the control in the base get_queryset.

Too many blank lines.

Too many blank lines.

Diff contents should always be binary.
This function should assert this.

Same here.

Should be a :py:class: reference.

Missing "Returns".

This should ideally be consistent in implicit vs. explicit comparisons.

Most docs use "The HTTP request from the client."

Missing a description.

Same description as above.

Do we want to allow for empty strings? Maybe just check truthiness?

Same as above.

Missing a description.

Been noticing lately that your test functions are all backwards. The basic case should come first. The variations come after. Can you reorder these to be consistent with all our other tests?

Can you explicitly say "with commit-id=", so it's clear what this is testing?

Here, too.

Should test that the results are what we expect.

All the same comments from the previous file, regarding docstrings and ordering, apply here as well.

Should check the results.

"review request revision" is weird

This isn't something the review request resource cares about.

Doesn't this break? It seems like we're no longer limiting the results to the correct diff revision.

This is incorrect, because this method ignores the diffset revision (which is filtered for in the caller).
Can we just remove the diff_revision parameter entirely from this (since it's not used) and fix the docstring here to explain that it's returning all diffsets linked to the review request?

"review request revision" isn't a thing

Missing a "the".

This should either be an assert statement or we should raise something like a ValueError. I'd prefer the latter.

Is the second part of this logic correct? That should trigger if neither is provided (which the assertion says is okay), but won't trigger if only one is provided. Seems this would be more correct/readable:

if ((committer_name or commiter_email) and
    not (committer_name and committer_email)):



>>> ((True or False) and not (True and False))
True
>>> ((False or True) and not (False and True))
True
>>> ((True or True) and not (True and True))
False
>>> ((False or False) and not (False and False))
False


Unit tests need to be fleshed out for all conditions.

This isn't in the function signature. It can be removed here.

Ship It!