Add support to provide review_request_id as URL

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

Information

RBTools
release-1.0.x
57db8f9...

Reviewers

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. Show all issues

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

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

    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)
     
     
    Show all issues

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

misery
misery
misery
david
  1. Going to make some small changes to docstrings and add support for rbt land, then get this in. Thanks!

  2. 
      
misery
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (66ae00a)