Add support to provide review_request_id as URL

Review Request #11107 — Created July 31, 2020 and updated

misery
RBTools
release-1.0.x
57db8f9...
rbtools
If a developer (or CI) gets a review request to review he
will get a whole URL because it was copy+pasted from browser.
This change allows to easily use that URL to get the patch
of given review request.

Provided an URL to rbt patch and saw that it extract the
review_request_id and the diff_id correctly and saw that
it correctly used the host.

Also provided an URL as interdiff format /4-5/ and
saw it was refused.

Description From Last Updated

How about making a reusable function in rbtools/utils/review_request.py (parse_review_request_url())?

chipx86chipx86

This should be utilizing the six module's compatibility imports. At the top of the file, in the correct import group ...

chipx86chipx86

I've been mulling this over. I think rather than make an assumption about the last bit of a URL containing ...

chipx86chipx86

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F821 undefined name 'm'

reviewbotreviewbot

F821 undefined name 'm'

reviewbotreviewbot

F821 undefined name 'm'

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot
chipx86
  1. This is a good idea, and I could see it being useful outside of rbt patch (rbt land would benefit from it).

    I have some suggestions for enhancing the way this works. Let me know if you want us to take it on.

  2. How about making a reusable function in rbtools/utils/review_request.py (parse_review_request_url())?

  3. rbtools/commands/patch.py (Diff revision 1)
     
     
     
     
     
     

    This should be utilizing the six module's compatibility imports. At the top of the file, in the correct import group (and organized alphabetically), you'd want:

    from django.utils.six.moves.urllib.parse import urlparse
    
  4. rbtools/commands/patch.py (Diff revision 1)
     
     

    I've been mulling this over. I think rather than make an assumption about the last bit of a URL containing an ID, the aforementioned function for this should pattern match using a regex.

    That would look something like:

    re.compile(r'^(?<server_url>https?://.*/(?:/s/[^/]+/)?)r/(?<review_request_id>\d+)/?')
    

    This would happen URLs with/without a subdomain, with/without a Local Site, and would capture even if passing in a diff URL.

    This could potentially be extended to even look for a diff/(\d+)/, working as an alternative to providing --diff-revision. Though that's certainly not something that needs to be implemented in this change.

    1. Thanks, that was a good idea. I switched to your regex and added diff_id as an optional attribute.
      Feel free to adopt this change for rbt land. :-)

  5. 
      
misery
Review request changed

Change Summary:

Switched to regex and added diff_id as group

Commit:

-fb063a852381260110f0a5ec7f6d317416b95bca
+4cbb3d98e960d85169ce5c8e287bbe3c4f995d75

Diff:

Revision 2 (+29 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

misery
misery
misery
Review request changed

Change Summary:

Allow --diff-revision if URL is provided.

Commit:

-52b8a21dac60c4de8c7ffb0f92465dcf39028623
+57db8f904c611be96ad8990cdb9b144b0b156a0e

Diff:

Revision 4 (+30 -1)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...