'rbt stamp': Adding the review request URL to the commit message

Review Request #6623 — Created Nov. 22, 2014 and submitted

nicolexin
RBTools
master
cdd98e0...
rbtools, students

Add a new rbt command called 'rbt stamp' that can automatically find the review request URL for the current commit and amend the commit with the right "Reviewed at" line.

Two cases:
- By default, 'rbt stamp' will guess the review request id using the same mechanism as 'rbt post -u'. After a review request id successfully found, it then stamp the last commit message with url of the review request. If there is no matching review request, it will abort and show the error message.

  • When a '-r' option is used, 'rbt stamp' will first check if it is a valid review request id owned by user. If yes, it will stamp the last commit message with url of the review request. Otherwise, it will abort and show the error message.

In either case, a comfirmation and new commit message will be print if success.

Note:
- Currently 'rbt stamp' support git only.
- If the current directory is not clean (i.e. since last commit user has made changes), 'rbt stamp' would include any changes made after the last commit into the new commit.

Testing manually (all passed):

  1. rbt stamp guessing review request id succeed: comfirmation with new commit message (SUCCESS)
  2. rbt stamp guessing review request id failed: abort with review request message (FAIL)
  3. rbt stamp with option -r valid review request id: comfirmation with review request message (SUCCESS)
  4. rbt stamp with option -r review request not owned by user: throwa api no permission error (FAIL)
  5. rbt stamp with option -r review request not exist: throws api not exist error (FAIL)
Description From Last Updated

Col: 34 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 1 E265 block comment should start with '# '

reviewbotreviewbot

'get_review_request' imported but unused

reviewbotreviewbot

undefined name 'summary'

reviewbotreviewbot

undefined name 'description'

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

'os' imported but unused

reviewbotreviewbot

undefined name 'tool'

reviewbotreviewbot

Let's remove the "(Git only)" bit. Instead, rbt stamp should display an error when run on an SCM that doesn't ...

chipx86chipx86

"Guesses" and "ID".

chipx86chipx86

"ID" and "by the user"

chipx86chipx86

Let's use single quotes instead of double.

chipx86chipx86

Space after "to", or the help text will end up saying "tobe stamped"

chipx86chipx86

This is also something that exists in rbt post. Can we pull this out into something reusable as well?

chipx86chipx86

Same with this and the other related functions.

chipx86chipx86

"URL"

chipx86chipx86

There should be 2 blank lines between the description and the "Reviewed at", and the "Reviewed at" cannot have a ...

chipx86chipx86

I'm not sure this is very useful to include here. We can remove this line.

chipx86chipx86

With the above comment, we'd be able to test for this early and bail with a suitable error.

chipx86chipx86

'get_user' imported but unused

reviewbotreviewbot

'InvalidRevisionSpecError' imported but unused

reviewbotreviewbot

Col: 33 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 49 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 37 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 37 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 33 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Instead of adding can_amend_commit = False on each subclass of SCMClient, why not add it to the base SCMClient class ...

brenniebrennie

I'd rather the SCMClients not document what commands will use their functions. We very likely will have other tools down ...

chipx86chipx86

Since it's not clear what the arguments are for the Falses, can we use keyword arguments? Like: guess_existing...(..., summary=False, ...)

chipx86chipx86

I realize now that this function doesn't really benefit from being outside of the class, since it's just a small ...

chipx86chipx86

This isn't necessarily used by rbt stamp. The caller should be responsible for displaying an error, rather than this.

chipx86chipx86

This isn't part of a class, so it shouldn't have self. What I'd suggest is re-working this to take cmd_args ...

chipx86chipx86

This is indented too far.

chipx86chipx86

Same comment here. I'd suggest having this take repository_name, summary, and description parameters.

chipx86chipx86

summary and description sound like strings. Can we use guess_summary and guess_description?

chipx86chipx86

These are pretty specific to their callers, and more will be added later. How about instead, the function takes a ...

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        setup.py
        rbtools/commands/stamp.py
        rbtools/commands/main.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        setup.py
        rbtools/commands/stamp.py
        rbtools/commands/main.py
    
    
  2. rbtools/commands/main.py (Diff revision 1)
     
     
    Col: 34
     E703 statement ends with a semicolon
    
  3. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 1
     E265 block comment should start with '# '
    
  4. rbtools/commands/stamp.py (Diff revision 1)
     
     
     'get_review_request' imported but unused
    
  5. rbtools/commands/stamp.py (Diff revision 1)
     
     
     undefined name 'summary'
    
  6. rbtools/commands/stamp.py (Diff revision 1)
     
     
     undefined name 'description'
    
  7. rbtools/commands/stamp.py (Diff revision 1)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  8. 
      
NI
NI
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/commands/stamp.py
        setup.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/commands/stamp.py
        setup.py
        rbtools/clients/git.py
    
    
  2. rbtools/commands/stamp.py (Diff revision 2)
     
     
     'os' imported but unused
    
  3. rbtools/commands/stamp.py (Diff revision 2)
     
     
     undefined name 'tool'
    
  4. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/commands/stamp.py
        setup.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/commands/stamp.py
        setup.py
        rbtools/clients/git.py
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/stamp.py (Diff revision 3)
     
     

    Let's remove the "(Git only)" bit. Instead, rbt stamp should display an error when run on an SCM that doesn't support this.

    To do that, what I'd suggest is adding a new capability variable to SCMClient: can_amend_commit. (This is the only thing that we need that is currently Git-only, right?)

    rbt stamp can then check that once we know what SCMClient we're working with, and if it's False, we can print an error like: "rbt stamp is not available for this type of repository."

  3. rbtools/commands/stamp.py (Diff revision 3)
     
     

    "Guesses" and "ID".

  4. rbtools/commands/stamp.py (Diff revision 3)
     
     

    "ID" and "by the user"

  5. rbtools/commands/stamp.py (Diff revision 3)
     
     
     

    Let's use single quotes instead of double.

  6. rbtools/commands/stamp.py (Diff revision 3)
     
     
     

    Space after "to", or the help text will end up saying "tobe stamped"

  7. rbtools/commands/stamp.py (Diff revision 3)
     
     

    This is also something that exists in rbt post. Can we pull this out into something reusable as well?

  8. rbtools/commands/stamp.py (Diff revision 3)
     
     

    Same with this and the other related functions.

  9. rbtools/commands/stamp.py (Diff revision 3)
     
     
  10. rbtools/commands/stamp.py (Diff revision 3)
     
     
     
     
     
     

    There should be 2 blank lines between the description and the "Reviewed at", and the "Reviewed at" cannot have a colon. Also, there must be a blank line at the end.

    You can see patch.py for how it builds its message.

  11. rbtools/commands/stamp.py (Diff revision 3)
     
     

    I'm not sure this is very useful to include here. We can remove this line.

  12. rbtools/commands/stamp.py (Diff revision 3)
     
     
     

    With the above comment, we'd be able to test for this early and bail with a suitable error.

  13. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/commands/stamp.py
        rbtools/commands/post.py
        rbtools/clients/plastic.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        setup.py
        rbtools/clients/svn.py
        rbtools/utils/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/commands/stamp.py
        rbtools/commands/post.py
        rbtools/clients/plastic.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        setup.py
        rbtools/clients/svn.py
        rbtools/utils/review_request.py
    
    
  2. rbtools/commands/post.py (Diff revision 4)
     
     
     'get_user' imported but unused
    
  3. rbtools/commands/stamp.py (Diff revision 4)
     
     
     'InvalidRevisionSpecError' imported but unused
    
  4. rbtools/utils/review_request.py (Diff revision 4)
     
     
    Col: 33
     E127 continuation line over-indented for visual indent
    
  5. rbtools/utils/review_request.py (Diff revision 4)
     
     
    Col: 29
     E127 continuation line over-indented for visual indent
    
  6. rbtools/utils/review_request.py (Diff revision 4)
     
     
    Col: 49
     E127 continuation line over-indented for visual indent
    
  7. rbtools/utils/review_request.py (Diff revision 4)
     
     
    Col: 37
     E126 continuation line over-indented for hanging indent
    
  8. rbtools/utils/review_request.py (Diff revision 4)
     
     
    Col: 37
     E126 continuation line over-indented for hanging indent
    
  9. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/commands/stamp.py
        rbtools/commands/post.py
        rbtools/clients/plastic.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        setup.py
        rbtools/clients/svn.py
        rbtools/utils/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/commands/stamp.py
        rbtools/commands/post.py
        rbtools/clients/plastic.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        setup.py
        rbtools/clients/svn.py
        rbtools/utils/review_request.py
    
    
  2. rbtools/utils/review_request.py (Diff revision 5)
     
     
    Col: 33
     E127 continuation line over-indented for visual indent
    
  3. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/commands/stamp.py
        rbtools/commands/post.py
        rbtools/clients/plastic.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        setup.py
        rbtools/clients/svn.py
        rbtools/utils/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/commands/stamp.py
        rbtools/commands/post.py
        rbtools/clients/plastic.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        setup.py
        rbtools/clients/svn.py
        rbtools/utils/review_request.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/clients/bazaar.py (Diff revision 6)
     
     

    Instead of adding can_amend_commit = False on each subclass of SCMClient, why not add it to the base SCMClient class and then only override it for clients that do support it? That way, a subclass of SCMClient is guaranteed to have a value for client.can_ammend_commit, even if the author of the class doesn't specify if the commit can be amended.

    1. Yeah, this will also future-proof any new SCMClients.

  3. 
      
chipx86
  1. Looking good! A few things we need done here first, but I think we're very close to landing this.

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

    I'd rather the SCMClients not document what commands will use their functions. We very likely will have other tools down the road want to use this, and it'll be confusing to the authors if they think this is reserved for rbt stamp.

    Also, "This modifies," rather than "This modify."

  3. rbtools/utils/review_request.py (Diff revision 6)
     
     

    I realize now that this function doesn't really benefit from being outside of the class, since it's just a small wrapper around a couple other functions. We can move this one back into the class.

  4. rbtools/utils/review_request.py (Diff revision 6)
     
     

    This isn't part of a class, so it shouldn't have self.

    What I'd suggest is re-working this to take cmd_args and tool parameters. The caller in the SCMClient should then handle the whole hasattr(self, '_revisions') logic itself, but would then call this function with its cmd_args and tool.

  5. rbtools/utils/review_request.py (Diff revision 6)
     
     
     

    Same comment here. I'd suggest having this take repository_name, summary, and description parameters.

  6. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        rbtools/commands/stamp.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        rbtools/commands/stamp.py
        setup.py
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/stamp.py (Diff revisions 6 - 7)
     
     

    Since it's not clear what the arguments are for the Falses, can we use keyword arguments? Like:

    guess_existing...(..., summary=False, ...)
    
  3. rbtools/utils/review_request.py (Diff revisions 6 - 7)
     
     
     

    This isn't necessarily used by rbt stamp. The caller should be responsible for displaying an error, rather than this.

  4. rbtools/utils/review_request.py (Diff revisions 6 - 7)
     
     

    This is indented too far.

  5. rbtools/utils/review_request.py (Diff revisions 6 - 7)
     
     

    summary and description sound like strings. Can we use guess_summary and guess_description?

  6. rbtools/utils/review_request.py (Diff revisions 6 - 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These are pretty specific to their callers, and more will be added later. How about instead, the function takes a is_fuzzy_match_func, instead of taking options. The caller can then provide a reference to a functio that does the question = and if confirm(), and will return True (to cause review_requesst.id to be returned) or False (to skip).

    So like:

    def guess_existing_review_request_id(..., is_fuzzy_match_func=None):
        ...
    
        for score, review_request in possible_matches:
            if ((score.is_exact_match() and exact_match_count == 1) or
                (callable(is_fuzzy_match_func) and
                 is_fuzzy_match_func(review_request))):
                return review_request.id
    

    Then the caller can do:

    class Stamp(object):
        ...
    
        def _ask_review_request_match(self, review_request):
            question = ('Stamp last ... ' % (...))
    
            return confirm(
                "Stamp last commit with Review Request #%s: '%s'? "
                % (...))
    
        def whatever(self):
            ...
    
            guess_existing_review_request_id(
                ...,
                is_fuzzy_match_func=self._ask_review_request_match)
    

    Same with the rbt post command.

    That way, the caller has complete control over the presentation, and the utility function gets to remain simple. Callers could then even call this with some specialized matching without a confirm() call, if needed.

  7. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        rbtools/commands/stamp.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/review_request.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
        rbtools/commands/stamp.py
        setup.py
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
NI
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (157e416)
Loading...