'rbt stamp': Adding the review request URL to the commit message
Review Request #6623 — Created Nov. 22, 2014 and submitted
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):
- 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)
| Description | From | Last Updated |
|---|---|---|
|
Col: 34 E703 statement ends with a semicolon |
 reviewbot
|
|
|
Col: 1 E265 block comment should start with '# ' |
reviewbot
|
|
|
'get_review_request' imported but unused |
reviewbot
|
|
|
undefined name 'summary' |
reviewbot
|
|
|
undefined name 'description' |
reviewbot
|
|
|
Col: 9 E265 block comment should start with '# ' |
reviewbot
|
|
|
'os' imported but unused |
reviewbot
|
|
|
undefined name 'tool' |
reviewbot
|
|
|
Let's remove the "(Git only)" bit. Instead, rbt stamp should display an error when run on an SCM that doesn't … |
 chipx86
|
|
|
"Guesses" and "ID". |
chipx86
|
|
|
"ID" and "by the user" |
chipx86
|
|
|
Let's use single quotes instead of double. |
chipx86
|
|
|
Space after "to", or the help text will end up saying "tobe stamped" |
chipx86
|
|
|
This is also something that exists in rbt post. Can we pull this out into something reusable as well? |
chipx86
|
|
|
Same with this and the other related functions. |
chipx86
|
|
|
"URL" |
chipx86
|
|
|
There should be 2 blank lines between the description and the "Reviewed at", and the "Reviewed at" cannot have a … |
chipx86
|
|
|
I'm not sure this is very useful to include here. We can remove this line. |
chipx86
|
|
|
With the above comment, we'd be able to test for this early and bail with a suitable error. |
chipx86
|
|
|
'get_user' imported but unused |
reviewbot
|
|
|
'InvalidRevisionSpecError' imported but unused |
reviewbot
|
|
|
Col: 33 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 29 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 49 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 37 E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Col: 37 E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Col: 33 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Instead of adding can_amend_commit = False on each subclass of SCMClient, why not add it to the base SCMClient class … |
 brennie
|
|
|
I'd rather the SCMClients not document what commands will use their functions. We very likely will have other tools down … |
chipx86
|
|
|
Since it's not clear what the arguments are for the Falses, can we use keyword arguments? Like: guess_existing...(..., summary=False, ...) |
chipx86
|
|
|
I realize now that this function doesn't really benefit from being outside of the class, since it's just a small … |
chipx86
|
|
|
This isn't necessarily used by rbt stamp. The caller should be responsible for displaying an error, rather than this. |
chipx86
|
|
|
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
|
|
|
This is indented too far. |
chipx86
|
|
|
Same comment here. I'd suggest having this take repository_name, summary, and description parameters. |
chipx86
|
|
|
summary and description sound like strings. Can we use guess_summary and guess_description? |
chipx86
|
|
|
These are pretty specific to their callers, and more will be added later. How about instead, the function takes a … |
chipx86
|
NI
Summary: |
|
|||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+263 -68) |
-
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 -
-
NI
Testing Done: |
|
|||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 3 (+261 -68) |
-
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) 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) Space after "to", or the help text will end up saying "tobe stamped"
-
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?
-
-
-
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.
-
rbtools/commands/stamp.py (Diff revision 3) I'm not sure this is very useful to include here. We can remove this line.
-
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.
NI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+294 -175) |
-
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 4) Col: 33 E127 continuation line over-indented for visual indent
-
rbtools/utils/review_request.py (Diff revision 4) Col: 29 E127 continuation line over-indented for visual indent
-
rbtools/utils/review_request.py (Diff revision 4) Col: 49 E127 continuation line over-indented for visual indent
-
rbtools/utils/review_request.py (Diff revision 4) Col: 37 E126 continuation line over-indented for hanging indent
-
rbtools/utils/review_request.py (Diff revision 4) Col: 37 E126 continuation line over-indented for hanging indent
NI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+293 -176) |
-
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) Col: 33 E127 continuation line over-indented for visual indent
NI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+293 -176) |
-
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) Instead of adding
can_amend_commit = Falseon each subclass of SCMClient, why not add it to the baseSCMClientclass and then only override it for clients that do support it? That way, a subclass of SCMClient is guaranteed to have a value forclient.can_ammend_commit, even if the author of the class doesn't specify if the commit can be amended.
-
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) 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) 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) This isn't part of a class, so it shouldn't have
self.What I'd suggest is re-working this to take
cmd_argsandtoolparameters. The caller in the SCMClient should then handle the wholehasattr(self, '_revisions')logic itself, but would then call this function with itscmd_argsandtool. -
rbtools/utils/review_request.py (Diff revision 6) Same comment here. I'd suggest having this take
repository_name,summary, anddescriptionparameters.
NI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+280 -176) |
-
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) 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) 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) summaryanddescriptionsound like strings. Can we useguess_summaryandguess_description? -
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 takingoptions. The caller can then provide a reference to a functio that does thequestion =andif confirm(), and will returnTrue(to causereview_requesst.id to be returned) orFalse(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.idThen 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.
NI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+292 -173) |
-
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
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