Post review requests with history when supported

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

brennie
RBTools
master
9921
db94fcb...
rbtools

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.

  • 6
  • 0
  • 53
  • 3
  • 62
Description From Last Updated
Need to add with_parent_diff here. david david
These seem like artificial limitations that should probably not exist david david
The long command-line arg is just --include david david
The long command-line arg is just --exclude david david
These error messages seem highly internal. Is this really what we want to show to a user? (When might this ... david david
Grammar here is weird. How about "One of "diff_history" or "squashed_diff" must be provided to "Post.post_request()." david david
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
Review request changed

Change Summary:

Addressed feedback.

Commit:

-1a0d32a42a2742d9028bd403e3fd3a922ee4f87a
+db94fcbd7e104c61dda0b105e4903ebffa2cab8b

Diff:

Revision 15 (+897 -206)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
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. 
      
Loading...