Post review requests with history when supported

Review Request #9911 — Created May 4, 2018 and submitted

Information

RBTools
master
d00641c...

Reviewers

When the Review Board server has the review_request.supports_history
capability, we will default to posting with history. Using history can
be forced with the new -H/--with-history flag, which will cause an
error if the server does not support it.

Traditional-style squashed review requests are still supported: they can
be posted with -S/--squash-history.

Review requests that were created with history will always be updated
with history and review requests not created with history will,
likewise, never be updated with history. Since review requests in
Review Board exist in one state or the other, attempting to post would
result in an error, so we do not let users attempt such posts.

If the server does not support history, the -S and -H options are
ignored.

Posted this review request.
Posted a review request with multiple commits to my devserver.

Description From Last Updated

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F821 undefined name 'query_args'

reviewbotreviewbot

F841 local variable 'query_args' is assigned to but never used

reviewbotreviewbot

Docstring?

daviddavid

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

daviddavid

typo: Attrirbutes

daviddavid

commandline -> command line

daviddavid

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

daviddavid

Commandline -> Command line

daviddavid

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

daviddavid

Can we wrap the arg value in parens?

daviddavid

commandline -> command line

daviddavid

commandline -> command line

daviddavid

Wrapping is a little weird here.

daviddavid

E501 line too long (81 > 79 characters)

reviewbotreviewbot

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 …

chipx86chipx86

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

chipx86chipx86

Byte string prefix?

chipx86chipx86

Can we simplify the language and say "cannot both be provided" instead of "mutually exclusive?" The latter's going to be …

chipx86chipx86

These would be nice to implement though. In fact, having history enabled by default and not allowing filtering is going …

chipx86chipx86

I don't really like doing this sort of thing. It'd be much nicer to have some proper checks with proper …

chipx86chipx86

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

chipx86chipx86

This was correct before.

chipx86chipx86

Typo: "hvae" -> "have".

chipx86chipx86

Can you call this review_request_id and review_request_url?

chipx86chipx86

Too many "the review" parts in here.

chipx86chipx86

Can we make additional_fields a list of strings? Wishing we did that with only_links and only_fields in the API calls, …

chipx86chipx86

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

F821 undefined name 'review_url'

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

Two blank lines.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Alphabetical order.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

These should be the same import group.

chipx86chipx86

Blank line between these.

chipx86chipx86

These can start on the raise line.

chipx86chipx86

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(...) …

chipx86chipx86

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

chipx86chipx86

bool

chipx86chipx86

You can do: only_fields = [ ... ] + additional_fields

chipx86chipx86

These can be combined into one statement.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Let's set to None in an else.

chipx86chipx86

Use +=. It's faster.

chipx86chipx86

F821 undefined name 'Noen'

reviewbotreviewbot

F821 undefined name 'Noen'

reviewbotreviewbot

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

rbtools.clients.errors.SCMError.

chipx86chipx86

Need to add with_parent_diff here.

daviddavid

These seem like artificial limitations that should probably not exist

daviddavid

The long command-line arg is just --include

daviddavid

The long command-line arg is just --exclude

daviddavid

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

daviddavid

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

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. rbtools/api/resource.py (Diff revision 2)
     
     

    Docstring?

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

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

    1. This was from a TODO comment to myself that accidentally got left in. Oopsie.

  4. rbtools/commands/post.py (Diff revision 2)
     
     

    typo: Attrirbutes

  5. rbtools/commands/post.py (Diff revision 2)
     
     

    commandline -> command line

  6. rbtools/commands/post.py (Diff revision 2)
     
     

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

  7. rbtools/commands/post.py (Diff revision 2)
     
     

    Commandline -> Command line

  8. rbtools/commands/post.py (Diff revision 2)
     
     

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

  9. rbtools/commands/post.py (Diff revision 2)
     
     

    Can we wrap the arg value in parens?

  10. rbtools/commands/post.py (Diff revision 2)
     
     

    commandline -> command line

  11. rbtools/commands/post.py (Diff revision 2)
     
     

    commandline -> command line

  12. rbtools/commands/post.py (Diff revision 2)
     
     
     

    Wrapping is a little weird here.

  13. 
      
brennie
brennie
brennie
brennie
Review request changed

Change Summary:

Send parent diff with all commits

Commit:

-ea75b0c4650d784adca7da9d4887e0d5edbc04bf
+482266166cb32af5b32fde7cbef6ef02831b7a51

Diff:

Revision 6 (+817 -193)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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.

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

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

  4. rbtools/clients/git.py (Diff revision 7)
     
     
     

    Byte string prefix?

  5. rbtools/commands/post.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

  6. rbtools/commands/post.py (Diff revision 7)
     
     
     
     
     
     
     
     
     

    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?

    1. Imagine a history A -> B -> C:

      A: adds foo
      B: renames foo to bar
      C: modifies bar

      If we attempt to post A^..C with -X foo, then the patch for C will fail to apply.

  7. rbtools/commands/post.py (Diff revision 7)
     
     

    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.

  8. rbtools/commands/post.py (Diff revision 7)
     
     

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

  9. rbtools/commands/post.py (Diff revision 7)
     
     

    This was correct before.

  10. rbtools/commands/post.py (Diff revision 7)
     
     

    Typo: "hvae" -> "have".

  11. rbtools/commands/post.py (Diff revision 7)
     
     

    Can you call this review_request_id and review_request_url?

  12. rbtools/commands/post.py (Diff revision 7)
     
     

    Too many "the review" parts in here.

  13. 
      
chipx86
  1. 
      
  2. rbtools/utils/review_request.py (Diff revision 7)
     
     

    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.

  3. 
      
brennie
Review request changed

Change Summary:

addressed Christian's issues.

Commit:

-87778e749e91f77c60adfcc0c83f57cc3a12b67a
+f7d491d310797df6e4a7796a7a93251c29e446cb

Diff:

Revision 8 (+851 -205)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
chipx86
  1. 
      
  2. rbtools/api/resource.py (Diff revision 11)
     
     
     
     

    Two blank lines.

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

    Alphabetical order.

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

    Alphabetical order.

  5. rbtools/commands/post.py (Diff revision 11)
     
     
     
     

    These should be the same import group.

  6. rbtools/commands/post.py (Diff revision 11)
     
     
     

    Blank line between these.

  7. rbtools/commands/post.py (Diff revision 11)
     
     
     
     

    These can start on the raise line.

    1. The string will take two lines, so it ends up being the same number of lines.

  8. rbtools/commands/post.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

    1. The only test we are repeating is just a boolean. It isn't really wasted computational power.

  9. rbtools/commands/post.py (Diff revision 11)
     
     

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

  10. rbtools/commands/post.py (Diff revision 11)
     
     
  11. rbtools/commands/post.py (Diff revision 11)
     
     
     

    You can do:

    only_fields = [
        ...
    ] + additional_fields
    
    1. Not if additional_fields is a tuple or any sort of non-list iterable.

    2. Then += would be better.

  12. rbtools/commands/post.py (Diff revision 11)
     
     
     

    These can be combined into one statement.

  13. rbtools/commands/post.py (Diff revision 11)
     
     

    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.

  14. rbtools/commands/post.py (Diff revision 11)
     
     
     

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

  15. rbtools/commands/post.py (Diff revision 11)
     
     
     
     
     

    Let's set to None in an else.

  16. rbtools/utils/review_request.py (Diff revision 11)
     
     

    Use +=. It's faster.

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

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

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

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

  4. 
      
brennie
Review request changed

Change Summary:

addressed Christian's issues.

Commit:

-5ba26797395602ef059824ec051919b8e1697764
+02a0177b1d44fcc695b2786c89d66c9eaa2a8091

Diff:

Revision 12 (+860 -206)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Change Summary:

More feedback addressed

Commit:

-02a0177b1d44fcc695b2786c89d66c9eaa2a8091
+0a35f0643ca7661532466bfb7c7bc85d8975a271

Diff:

Revision 13 (+896 -206)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

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

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

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

  4. rbtools/clients/git.py (Diff revision 14)
     
     

    This wasn't updated to be get_commit_history.

    Make sure the latest iteration of changes has been fully tested.

    1. I ended up also not changing the call site, only the method in the base client.

  5. rbtools/clients/git.py (Diff revision 14)
     
     

    rbtools.clients.errors.SCMError.

  6. 
      
brennie
david
  1. 
      
  2. rbtools/clients/git.py (Diff revision 15)
     
     
     
     

    Need to add with_parent_diff here.

  3. rbtools/commands/post.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
     
     
     
     

    These seem like artificial limitations that should probably not exist

  4. rbtools/commands/post.py (Diff revision 15)
     
     

    The long command-line arg is just --include

  5. rbtools/commands/post.py (Diff revision 15)
     
     

    The long command-line arg is just --exclude

  6. rbtools/commands/post.py (Diff revision 15)
     
     
     
     
     
     

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

    1. These are internal logic errors (unless someone is piggy-backing their custom command on top of rbt post) and are more like assertions. Should these be AssertionError instead?

  7. rbtools/commands/post.py (Diff revision 15)
     
     
     

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

  8. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (e9bb59a)
Loading...