Summary

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

Review Request #6623 — Created Nov. 22, 2014 and submitted Dec. 3, 2014, 10:44 p.m.

Information

Owner

nicolexin

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

cdd98e0...

Reviewers

Groups

rbtools, students

People

Description

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 Done

Testing manually (all passed):

rbt stamp guessing review request id succeed: comfirmation with new commit message (SUCCESS)
rbt stamp guessing review request id failed: abort with review request message (FAIL)
rbt stamp with option -r valid review request id: comfirmation with review request message (SUCCESS)
rbt stamp with option -r review request not owned by user: throwa api no permission error (FAIL)
rbt stamp with option -r review request not exist: throws api not exist error (FAIL)

Issues

Description	From	Last Updated
Col: 34 E703 statement ends with a semicolon	reviewbot	Nov. 28, 2014, 12:30 p.m.
Col: 1 E265 block comment should start with '# '	reviewbot	Nov. 28, 2014, 12:30 p.m.
'get_review_request' imported but unused	reviewbot	Nov. 28, 2014, 12:30 p.m.
undefined name 'summary'	reviewbot	Nov. 28, 2014, 12:30 p.m.
undefined name 'description'	reviewbot	Nov. 28, 2014, 12:30 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Nov. 28, 2014, 12:30 p.m.
'os' imported but unused	reviewbot	Nov. 28, 2014, 12:34 p.m.
undefined name 'tool'	reviewbot	Nov. 28, 2014, 12:34 p.m.
Let's remove the "(Git only)" bit. Instead, rbt stamp should display an error when run on an SCM that doesn't …	chipx86	Dec. 1, 2014, 6:50 p.m.
"Guesses" and "ID".	chipx86	Dec. 1, 2014, 6:50 p.m.
"ID" and "by the user"	chipx86	Dec. 1, 2014, 6:50 p.m.
Let's use single quotes instead of double.	chipx86	Dec. 1, 2014, 6:50 p.m.
Space after "to", or the help text will end up saying "tobe stamped"	chipx86	Dec. 1, 2014, 6:50 p.m.
This is also something that exists in rbt post. Can we pull this out into something reusable as well?	chipx86	Dec. 1, 2014, 6:50 p.m.
Same with this and the other related functions.	chipx86	Dec. 1, 2014, 6:50 p.m.
"URL"	chipx86	Dec. 1, 2014, 6:50 p.m.
There should be 2 blank lines between the description and the "Reviewed at", and the "Reviewed at" cannot have a …	chipx86	Dec. 1, 2014, 6:50 p.m.
I'm not sure this is very useful to include here. We can remove this line.	chipx86	Dec. 1, 2014, 6:50 p.m.
With the above comment, we'd be able to test for this early and bail with a suitable error.	chipx86	Dec. 1, 2014, 6:51 p.m.
'get_user' imported but unused	reviewbot	Dec. 1, 2014, 6:59 p.m.
'InvalidRevisionSpecError' imported but unused	reviewbot	Dec. 1, 2014, 6:59 p.m.
Col: 33 E127 continuation line over-indented for visual indent	reviewbot	Dec. 1, 2014, 6:59 p.m.
Col: 29 E127 continuation line over-indented for visual indent	reviewbot	Dec. 1, 2014, 6:59 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	Dec. 1, 2014, 6:59 p.m.
Col: 37 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 1, 2014, 6:59 p.m.
Col: 37 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 1, 2014, 6:59 p.m.
Col: 33 E127 continuation line over-indented for visual indent	reviewbot	Dec. 1, 2014, 7:07 p.m.
Instead of adding can_amend_commit = False on each subclass of SCMClient, why not add it to the base SCMClient class …	brennie	Dec. 3, 2014, 3:49 p.m.
I'd rather the SCMClients not document what commands will use their functions. We very likely will have other tools down …	chipx86	Dec. 3, 2014, 3:49 p.m.
Since it's not clear what the arguments are for the Falses, can we use keyword arguments? Like: guess_existing...(..., summary=False, ...)	chipx86	Dec. 3, 2014, 8:13 p.m.
I realize now that this function doesn't really benefit from being outside of the class, since it's just a small …	chipx86	Dec. 3, 2014, 3:49 p.m.
This isn't necessarily used by rbt stamp. The caller should be responsible for displaying an error, rather than this.	chipx86	Dec. 3, 2014, 8:13 p.m.
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 …	chipx86	Dec. 3, 2014, 3:49 p.m.
This is indented too far.	chipx86	Dec. 3, 2014, 8:13 p.m.
Same comment here. I'd suggest having this take repository_name, summary, and description parameters.	chipx86	Dec. 3, 2014, 3:49 p.m.
summary and description sound like strings. Can we use guess_summary and guess_description?	chipx86	Dec. 3, 2014, 8:13 p.m.
These are pretty specific to their callers, and more will be added later. How about instead, the function takes a …	chipx86	Dec. 3, 2014, 8:13 p.m.

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

rbtools/commands/main.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 34
 E703 statement ends with a semicolon
```
rbtools/commands/post.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 1
 E265 block comment should start with '# '
```
rbtools/commands/stamp.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'get_review_request' imported but unused
```
rbtools/commands/stamp.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'summary'
```
rbtools/commands/stamp.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'description'
```
rbtools/commands/stamp.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

Groups:: students

Summary:: 'rbt stamp': Adding the review request URL to the commit message
[WIP] 'rbt stamp': Adding the review request URL to the commit message

Summary:

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

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

Description:

~		Add a new rbt command that can automatically find the review request URL for the current commit (in git) and amend the commit with the right "Reviewed at" line.
	~	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 Done:

~		No testing done yet.
	~	Testing manually (all passed):
	+
	+	rbt stamp guessing review request id succeed: comfirmation with new commit message (SUCCESS)
	+	rbt stamp guessing review request id failed: abort with review request message (FAIL)
	+	rbt stamp with option -r valid review request id: comfirmation with review request message (SUCCESS)
	+	rbt stamp with option -r review request not owned by user: throwa api no permission error (FAIL)
	+	rbt stamp with option -r review request not exist: throws api not exist error (FAIL)

Commit:

73ded82b085aaf31fdc867cd361f7b1f389b508f

84a645e5ecd8fa372bb28c3a30a0850f2e13cb29

Diff:

Revision 2 (+263 -68)

Show changes

	setup.py
	rbtools/clients/git.py
	rbtools/commands/post.py
	rbtools/commands/stamp.py
	rbtools/utils/review_request.py

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

rbtools/commands/stamp.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'os' imported but unused
```
rbtools/commands/stamp.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 undefined name 'tool'
```

Testing Done:

		Testing manually (all passed):

		rbt stamp guessing review request id succeed: comfirmation with new commit message (SUCCESS)
~		rbt stamp guessing review request id failed: abort with review request message (FAIL)
~		rbt stamp with option -r valid review request id: comfirmation with review request message (SUCCESS)
	~	rbt stamp guessing review request id failed: abort with review request message (FAIL)
	~	rbt stamp with option -r valid review request id: comfirmation with review request message (SUCCESS)
		rbt stamp with option -r review request not owned by user: throwa api no permission error (FAIL)
		rbt stamp with option -r review request not exist: throws api not exist error (FAIL)

Commit:

84a645e5ecd8fa372bb28c3a30a0850f2e13cb29

a0ce6b931bbaf488321ffc1cffa53378d7e949a6

Diff:

Revision 3 (+261 -68)

Show changes

	setup.py
	rbtools/clients/git.py
	rbtools/commands/post.py
	rbtools/commands/stamp.py
	rbtools/utils/review_request.py

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

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

The issue has been resolved. Show all issues

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."

rbtools/commands/stamp.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"Guesses" and "ID".
```
rbtools/commands/stamp.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"ID" and "by the user"
```
rbtools/commands/stamp.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Let's use single quotes instead of double.
```
rbtools/commands/stamp.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Space after "to", or the help text will end up saying "tobe stamped"
```

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

The issue has been resolved. Show all issues

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

rbtools/commands/stamp.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same with this and the other related functions.
```
rbtools/commands/stamp.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"URL"
```

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

The issue has been resolved. Show all issues

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.

rbtools/commands/stamp.py (Diff revision 3)
The issue has been resolved. Show all issues
```
I'm not sure this is very useful to include here. We can remove this line.
```

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

The issue has been resolved. Show all issues

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

Commit:

a0ce6b931bbaf488321ffc1cffa53378d7e949a6

49677a55c96a648540e79eca820ba368871c8a15

Diff:

Revision 4 (+294 -175)

Show changes

	setup.py
	rbtools/clients/bazaar.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	rbtools/clients/svn.py
	3 more

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

rbtools/commands/post.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 'get_user' imported but unused
```
rbtools/commands/stamp.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 'InvalidRevisionSpecError' imported but unused
```
rbtools/utils/review_request.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 33
 E127 continuation line over-indented for visual indent
```
rbtools/utils/review_request.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 29
 E127 continuation line over-indented for visual indent
```
rbtools/utils/review_request.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
rbtools/utils/review_request.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 37
 E126 continuation line over-indented for hanging indent
```
rbtools/utils/review_request.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 37
 E126 continuation line over-indented for hanging indent
```

Commit:

49677a55c96a648540e79eca820ba368871c8a15

ba428f94261669578124c94f0d943e62bda67235

Diff:

Revision 5 (+293 -176)

Show changes

	setup.py
	rbtools/clients/bazaar.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	rbtools/clients/svn.py
	3 more

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

rbtools/utils/review_request.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 33
 E127 continuation line over-indented for visual indent
```

Commit:

ba428f94261669578124c94f0d943e62bda67235

c326b7dff269e1c4cfb7cea1ec9481743b6a2765

Diff:

Revision 6 (+293 -176)

Show changes

	setup.py
	rbtools/clients/bazaar.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	rbtools/clients/svn.py
	3 more

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

rbtools/clients/bazaar.py (Diff revision 6)

The issue has been resolved. Show all issues

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.

chipx86 Dec. 2, 2014, 6:04 p.m.

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

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

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

The issue has been resolved. Show all issues

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."

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

The issue has been dropped. Show all issues

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.

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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

Commit:

c326b7dff269e1c4cfb7cea1ec9481743b6a2765

aaa69a7c66d5c7e630225d36344b346697f6a525

Diff:

Revision 7 (+280 -176)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py
	rbtools/commands/stamp.py
	rbtools/utils/review_request.py

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

rbtools/commands/stamp.py (Diff revisions 6 - 7)

The issue has been resolved. Show all issues

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

guess_existing...(..., summary=False, ...)

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

The issue has been resolved. Show all issues

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

rbtools/utils/review_request.py (Diff revisions 6 - 7)
The issue has been resolved. Show all issues
```
This is indented too far.
```

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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.

Commit:

aaa69a7c66d5c7e630225d36344b346697f6a525

cdd98e05e132efe836873237ec91ec4323dd9c51

Diff:

Revision 8 (+292 -173)

Show changes

	setup.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py
	rbtools/commands/stamp.py
	rbtools/utils/review_request.py

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (157e416)