Summary:

Add commit message template

Review Request #6833 — Created Jan. 27, 2015 and discarded Jan. 29, 2019, 1:40 p.m.

Information
Owner:	tienv
Repository:	RBTools
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	rbtools, students
People:

Description

Some RBT commands, like patch/land, allow users to commit with a commit message. Previously, this commit message would be hard coded with python strings, however adding the option to specify a user defined template would be more helpful.

The added option is --template <template_directory_or_name>, which could be any of:
- the template's directory
- the template's directory within the "TEMPLATES_DIR" folder, as specified in .reviewboardrc
- the name of the template, as specified in .reviewboardrc

If no template is specified, a default template is selected.
If no template exists under the argument, the user will be prompted for the default template.
As template design is up to the user, the code is not responsible for handling misconfigured templates.

Testing Done

I've tested with (rbt patch):

--template <existing_template_path>

--template <non_existant_template_path>

--template <existing_template in TEMPLATES_DIR>

--template <variable name in .reviewboardrc>

--template with invalid syntax (like not closing the if statement)

--template attempting to access invalid variables

--commit (no --template, which should render the default template)
Unit tests are available as well, but I don't know if they make sense tested that way.

Issues

Description	From	Last Updated
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 3:53 p.m.
This doesn't belong in this change.	brennie	Feb. 25, 2015, 10:32 p.m.
Import statements should go at the beginning of the file. This is grouped in the second group (third-party non-rbtools imports).	brennie	Jan. 30, 2015, 2:36 p.m.
I know this is a first draft, but perhaps a flag and/or config option would be a better place for …	brennie	Jan. 30, 2015, 2:36 p.m.
Col: 62 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 40 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:53 p.m.
Usually the pattern is except TemplateNotFound as e: Same below.	brennie	Jan. 30, 2015, 2:36 p.m.
You can remove these lines. No point to keep them commented because they will be in version control.	brennie	Jan. 30, 2015, 2:36 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 2, 2015, 3:53 p.m.
So Jinja2 templates aren't actually python files. This would be better marked as a .txt file because Review Bot is …	brennie	Jan. 30, 2015, 2:36 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 42 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:53 p.m.
You are setting variables here but using review_request.bugs_closed later. Why not just use review_request.foo for everything?	brennie	March 2, 2015, 7:57 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 50 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 52 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:53 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 44 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
You don't want two blank lines between summary and description.	brennie	Jan. 30, 2015, 2:36 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 11 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
You don't want two blank lines between description and testing done. You may want to put the blank line after …	brennie	Jan. 30, 2015, 2:36 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 19 E228 missing whitespace around modulo operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 1 E112 expected an indented block	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 11 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 33 E228 missing whitespace around modulo operator	reviewbot	Jan. 30, 2015, 2:36 p.m.
Col: 2 E225 missing whitespace around operator	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 1 E112 expected an indented block	reviewbot	Jan. 30, 2015, 2:36 p.m.
Again, you probably don't want two blank lines here.	brennie	Jan. 30, 2015, 2:36 p.m.
'pdb' imported but unused	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 66 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 40 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 2, 2015, 3:54 p.m.
undefined name 'default_templates_dir'	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 3:54 p.m.
There's some inconsistency in the blank lines here. If the 'testing done' field is present, there's plenty of space, but …	david	Feb. 2, 2015, 3:55 p.m.
os and pkg_resources are standard library modules, and so should be in their own group before six. six should be …	david	Feb. 2, 2015, 3:55 p.m.
These should be a single import statement and kept alphabetical: from jinja2 import (Environment, FileSystemLoader, StrictUndefined, TemplateError, TemplateNotFound, UndefinedError)	david	Feb. 2, 2015, 3:55 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 2, 2015, 3:54 p.m.
extract_template is a pretty weird name for this. How about render_commit_message_template?	david	Feb. 2, 2015, 3:55 p.m.
Col: 66 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 40 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:54 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
How about a message more like raise CommandError('Syntax error in template %s: %s' % (template_name, ue.message))	david	Feb. 2, 2015, 3:55 p.m.
I'd like to see this error message contain the template name, and put it all on one line. Something like …	david	Feb. 2, 2015, 3:55 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 2, 2015, 3:55 p.m.
This should probably check to make sure that the template name isn't absolute before joining it to cwd/templates	david	Feb. 2, 2015, 3:55 p.m.
undefined name 'default_templates_dir'	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (112 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 29 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 29 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 68 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 37 E127 continuation line over-indented for visual indent	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 57 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 100 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (111 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 33 E127 continuation line over-indented for visual indent	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 33 E127 continuation line over-indented for visual indent	reviewbot	Feb. 2, 2015, 3:55 p.m.
Col: 80 E501 line too long (102 > 79 characters)	reviewbot	Feb. 2, 2015, 3:55 p.m.
local variable 'e' is assigned to but never used	reviewbot	Feb. 2, 2015, 3:56 p.m.
Col: 37 E127 continuation line over-indented for visual indent	reviewbot	Feb. 2, 2015, 3:56 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 2, 2015, 3:56 p.m.
Col: 33 E127 continuation line over-indented for visual indent	reviewbot	Feb. 2, 2015, 3:56 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Feb. 2, 2015, 3:56 p.m.
Col: 42 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 3:56 p.m.
local variable 'e' is assigned to but never used	reviewbot	Feb. 3, 2015, 9:24 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 2, 2015, 8:11 p.m.
self.config.get(name) should be the equivalent of the self.config[name] if condition else None	VT VTL-Developer	Feb. 2, 2015, 8:12 p.m.
Col: 80 E501 line too long (98 > 79 characters)	reviewbot	Feb. 2, 2015, 8:11 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 2, 2015, 8:11 p.m.
Col: 31 E128 continuation line under-indented for visual indent	reviewbot	Feb. 2, 2015, 8:11 p.m.
Col: 33 E127 continuation line over-indented for visual indent	reviewbot	Feb. 2, 2015, 8:11 p.m.
I noticed you have three cases. For the case where there's only template_name, it get combines with working directory and …	VT VTL-Developer	Feb. 4, 2015, 4:02 p.m.
Col: 59 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 8:11 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Feb. 2, 2015, 8:11 p.m.
Col: 71 W291 trailing whitespace	reviewbot	Feb. 2, 2015, 8:11 p.m.
Add a real help description.	brennie	Feb. 4, 2015, 4:55 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 3, 2015, 9:24 p.m.
One blank line instead of two.	brennie	Feb. 3, 2015, 11:43 p.m.
Add a blank line between these two.	brennie	Feb. 3, 2015, 11:43 p.m.
Undo this change.	brennie	Feb. 4, 2015, 4:57 p.m.
We only use trailing commas on the last line of a dictionary definition.	brennie	Feb. 3, 2015, 11:43 p.m.
Move this up to the previous line.	brennie	Feb. 3, 2015, 11:44 p.m.
Use e instead of tnfe. This is for consistency with the rest of the codebase.	brennie	Feb. 3, 2015, 11:44 p.m.
Use e instead of ue.	brennie	Feb. 3, 2015, 11:44 p.m.
Use e instead of te.	brennie	Feb. 3, 2015, 11:44 p.m.
We don't use str.format. Please use % instead for string formatting.	brennie	Feb. 3, 2015, 11:44 p.m.
More than one of these can fit on a line.	brennie	Feb. 4, 2015, 4:03 p.m.
Blank line between a statement and a block	brennie	Feb. 4, 2015, 4:09 p.m.
We don't use str.format. Please use % for formatting instead.	brennie	Feb. 4, 2015, 4:09 p.m.
This string isn't formatted correctly. There will be no space between file and must. Also, please use all available line …	brennie	Feb. 4, 2015, 4:56 p.m.
Since you are doing the following: else: if foo: bar() else: baz() This can be changed to: elif foo: bar() …	brennie	Feb. 4, 2015, 4:10 p.m.
Isn't this the template filename, not the directory?	brennie	Feb. 4, 2015, 4:10 p.m.
One blank line between these.	brennie	Feb. 4, 2015, 4:11 p.m.
Why unset the name here?	brennie	Feb. 4, 2015, 4:11 p.m.
Use %s instead for string formatting.	brennie	Feb. 4, 2015, 4:11 p.m.
More than one of these can fit on a line.	brennie	Feb. 4, 2015, 4:11 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 4, 2015, 4:15 p.m.
'Template' imported but unused	reviewbot	Feb. 4, 2015, 4:15 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Feb. 4, 2015, 4:15 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Feb. 4, 2015, 4:15 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 4, 2015, 4:15 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 4, 2015, 5:09 p.m.
undefined name 'ue'	reviewbot	Feb. 4, 2015, 5:09 p.m.
Please revert this change.	david	Feb. 25, 2015, 10:15 p.m.
Please revert this change.	david	Feb. 25, 2015, 10:15 p.m.
This is part of a separate review request and should not be present in this change.	david	Feb. 25, 2015, 10:15 p.m.
How about just doing import jinja2 and then using qualified names (jinja2.Environment) for each of these? Many of these names …	david	Feb. 25, 2015, 10:18 p.m.
This should be a fully-qualified module name (rbtools.utils.console)	david	Feb. 25, 2015, 10:18 p.m.
This method should have a docstring.	david	March 2, 2015, 7:55 p.m.
Based on this code, it looks like directory is actually a filename, not a directory.	david	Feb. 25, 2015, 10:32 p.m.
Comments should have proper capitalization and punctuation.	david	Feb. 25, 2015, 10:32 p.m.
We generally don't use try/else. Instead, you can move template.render() into the try case above and then have all the …	david	March 2, 2015, 7:52 p.m.
Is this the best name for this method? Probably not.	david	Feb. 25, 2015, 10:32 p.m.
This doesn't look like it's returning the directory name (rather, it looks like it's returning a filename). The explanation of …	david	Feb. 25, 2015, 10:32 p.m.
This function isn't terribly well named anymore. Can we change it to be build_commit_message?	david	Feb. 25, 2015, 10:32 p.m.
'extract_commit_message' imported but unused	reviewbot	March 2, 2015, 7:53 p.m.
undefined name 'build_commit_message'	reviewbot	March 2, 2015, 7:53 p.m.
'extract_commit_message' imported but unused	reviewbot	March 2, 2015, 7:53 p.m.
undefined name 'build_commit_message'	reviewbot	March 2, 2015, 7:53 p.m.
Please revert this change.	david	March 9, 2015, 10:47 p.m.
Revert this change.	david	March 9, 2015, 10:47 p.m.
I'm not sure why you set these variables, given that later on you use review_request.bugs_closed. Can we just prefix all …	david	March 9, 2015, 10:50 p.m.
Should have a space before the closing %}	david	March 9, 2015, 10:48 p.m.
Should have a space before the closing %}	david	March 9, 2015, 10:48 p.m.
This should be a docstring, not a comment (and change 'Returns' to 'Return' and add a period at the end.)	david	March 9, 2015, 10:50 p.m.
Returns -> Return.	david	March 9, 2015, 10:50 p.m.
What is 'template_cmnd'? That string doesn't appear anywhere else in this change.	david	March 9, 2015, 10:52 p.m.
This should present an error that the commit message template directory could not be found.	david	March 10, 2015, 11:03 p.m.
Please add a class docstring.	david	March 10, 2015, 11:03 p.m.
Indent this 4 more spaces.	david	March 10, 2015, 11:03 p.m.
Please add docstrings for all the test methods in here. We want to print nice things with nosetests -v	david	March 10, 2015, 11:03 p.m.
Alphabetize these.	brennie	March 13, 2015, 7:34 p.m.
I would reword this to say something like "Use the specified template (either in the directory specified in .reviewboardrc or …	brennie	March 13, 2015, 7:38 p.m.
We use single quotes for strings.	brennie	March 13, 2015, 7:39 p.m.
This should be on the previous line.	brennie	March 13, 2015, 7:39 p.m.
Since we're using this in multiple places, this might be worth putting into a list in __init__.py and referencing it …	brennie	March 18, 2015, 3:10 p.m.
See my other comment.	brennie	March 13, 2015, 7:46 p.m.
We use single quotes for strings.	brennie	March 13, 2015, 7:46 p.m.
This should be on the previous line.	brennie	March 13, 2015, 7:47 p.m.
Use single quotes for all these strings.	brennie	March 13, 2015, 7:47 p.m.
Can you put the first %s in double quotes?	brennie	March 13, 2015, 7:47 p.m.
Use a %-formatting expression. Its more efficient than string concatenation.	brennie	March 13, 2015, 7:48 p.m.
Can you format that is as a reStructured Text List and mark the keys with double backticks, e.g. * ``foo``: …	brennie	March 13, 2015, 7:52 p.m.
Can we rename template_cmd to something like template_arg or specified_template? template_cmd seems like an inaccurate name for it.	brennie	March 13, 2015, 7:52 p.m.
Maybe this should this just be template ?	brennie	March 13, 2015, 7:54 p.m.
Blank line between these.	brennie	March 13, 2015, 7:55 p.m.
Put this on the previous line.	brennie	March 13, 2015, 7:55 p.m.
Use a %-formatting expression here.	brennie	March 13, 2015, 7:55 p.m.
Blank line between these.	brennie	March 13, 2015, 7:56 p.m.
Blank line between these.	brennie	March 13, 2015, 7:56 p.m.
Blank line between these.	brennie	March 13, 2015, 7:56 p.m.
Alphabetize these.	brennie	March 13, 2015, 8:10 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 18, 2015, 3:10 p.m.
Can you format the docstring as such: def render_template(filename, review_request): """Render a commit message template. It is up to the …	david	March 18, 2015, 3:46 p.m.
This should be a docstring, not a comment.	david	March 18, 2015, 3:47 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	March 18, 2015, 3:10 p.m.
No blank line between the docstring and code for methods.	david	March 18, 2015, 3:47 p.m.
All test docstrings should start with "Testing" instead of "Test". Here and below.	david	March 18, 2015, 3:51 p.m.
First line of a docstring should come right after """ ("""Render a template...)	david	March 20, 2015, 10:56 p.m.
This should just be """Return the filename of the default template.""" (note: one line, with period)	david	March 20, 2015, 10:56 p.m.
Instead of defining a new empty class for this, I think you could just do mock_review_request = object()	david	March 20, 2015, 10:58 p.m.
alphabetical order	VT VTL-Developer	March 30, 2015, 5:30 p.m.
If .reviewboardrc specifies a template, it should be used for all commits, without needing to pass an argument.	chipx86	March 31, 2015, 8:55 p.m.
This isn't needed.	chipx86	March 31, 2015, 8:22 p.m.
The wrapping should use up more of the line.	chipx86	March 31, 2015, 8:39 p.m.
Same comments here.	chipx86	March 31, 2015, 8:41 p.m.
This should be part of the next group of imports.	chipx86	March 31, 2015, 8:41 p.m.
This should use format strings, instead of string concatenation.	chipx86	March 31, 2015, 8:42 p.m.
Blank line between these.	chipx86	March 31, 2015, 9:01 p.m.
This is deprecated. You'll need to rewrite the code to use os.walk instead.	chipx86	March 31, 2015, 9:01 p.m.
Looks like trim_blocks will easily fit on the previous line.	chipx86	March 31, 2015, 9:01 p.m.
I don't know that a newline is appropriate here. Can it just be: 'Template error: %s' ?	chipx86	March 31, 2015, 9:01 p.m.
You can use os.path.abspath(template_arg) instead, and it will do the equivalent.	chipx86	March 31, 2015, 9:02 p.m.
This leaves out the space between sentences.	chipx86	April 1, 2015, 5 p.m.
This code is repeating itself. You should rewrite it to only do this once.	chipx86	April 1, 2015, 5 p.m.
Same here.	chipx86	April 1, 2015, 5:01 p.m.
Missing a trailing period.	chipx86	April 1, 2015, 5:01 p.m.
Missing a trailing period.	chipx86	April 1, 2015, 5:01 p.m.
Must be in alphabetical order.	chipx86	April 1, 2015, 5:01 p.m.
You shouldn't really use """ here. Instead, it's best to just do: mock_template = ( 'Start\n' '...\n' '...\n') That way, …	chipx86	April 1, 2015, 5:13 p.m.
These really should go in the tests themselves, so that they're self-contained and not vulnerable to impact from another test …	chipx86	April 2, 2015, 4:06 p.m.
This should be done in setUp().	chipx86	April 1, 2015, 5:27 p.m.
I missed this part of the design before. I realize now that this is allowing multiple templates per tree. I've …	chipx86	Sept. 12, 2015, 6:58 p.m.
No backticks here. It'll actually render incorrectly, as comments are meant to be in ReST syntax, where backticks are used …	chipx86	April 2, 2015, 4:10 p.m.
Might be nice to have a bit of information on the format of the template here. Things like how variables/functions …	chipx86	April 2, 2015, 4:18 p.m.
You should be able to use os.path.split here, to do this in one go.	chipx86	April 2, 2015, 4:19 p.m.
Missing a trailing period.	chipx86	April 2, 2015, 4:20 p.m.
Should use single quotes where possible.	chipx86	April 2, 2015, 4:20 p.m.
"Error" shouldn't be capitalized.	chipx86	April 2, 2015, 4:21 p.m.
We may want to check that this is a directory first, and provide an error otherwise.	chipx86	April 2, 2015, 4:24 p.m.
This should just be: if not default_templates: Better than checking < 1, since the only reasonable value is 0.	chipx86	April 2, 2015, 4:24 p.m.
Blank line here.	chipx86	April 2, 2015, 4:24 p.m.
I kind of think that if the template is missing, we should just error out. That's a very clear configuration …	chipx86	April 2, 2015, 4:30 p.m.
Missing a trailing period. "Raises" is maybe the wrong word here?	chipx86	April 2, 2015, 4:31 p.m.
'build_commit_message' imported but unused	reviewbot	Sept. 12, 2015, 7 p.m.
Col: 69 W291 trailing whitespace	reviewbot	Sept. 12, 2015, 7 p.m.
undefined name 'extract_commit_message'	reviewbot	Sept. 12, 2015, 7 p.m.
'build_commit_message' imported but unused	reviewbot	Sept. 12, 2015, 7:10 p.m.
Col: 69 W291 trailing whitespace	reviewbot	Sept. 12, 2015, 7:10 p.m.
undefined name 'extract_commit_message'	reviewbot	Sept. 12, 2015, 7:10 p.m.

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/utils/tien_template.py
    rbtools/utils/commands.py


WARNING: Number of comments exceeded maximum, showing 30 of 34.

rbtools/commands/post.py (Diff revision 1)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/utils/commands.py (Diff revision 1)
Show all issues
```
Col: 62
 W291 trailing whitespace
```

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)
Show all issues
```
Col: 40
 W291 trailing whitespace
```

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 42
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 50
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 52
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 44
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 11
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```

rbtools/utils/tien_template.py (Diff revision 1)

Show all issues

Col: 19
 E228 missing whitespace around modulo operator

rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 1
 E112 expected an indented block
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 11
 E225 missing whitespace around operator
```

rbtools/utils/tien_template.py (Diff revision 1)

Show all issues

Col: 33
 E228 missing whitespace around modulo operator

rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 2
 E225 missing whitespace around operator
```
rbtools/utils/tien_template.py (Diff revision 1)
Show all issues
```
Col: 1
 E112 expected an indented block
```

rbtools/commands/post.py (Diff revision 1)
Show all issues
```
This doesn't belong in this change.
```

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Import statements should go at the beginning of the file. This is grouped in the second group (third-party non-rbtools imports).

rbtools/utils/commands.py (Diff revision 1)

Show all issues

I know this is a first draft, but perhaps a flag and/or config option would be a better place for this. Something like

TEMPLATE_ENV = 'templates'
POST_TEMLATE = 'post.txt'

with some reasonable defaults. Or it could be passed in as a parameter to the function. I'm not sure about this.

rbtools/utils/commands.py (Diff revision 1)

Show all issues

Usually the pattern is

except TemplateNotFound as e:

Same below.

rbtools/utils/commands.py (Diff revision 1)

Show all issues

You can remove these lines. No point to keep them commented because they will be in version control.

rbtools/utils/tien_template.py (Diff revision 1)

Show all issues

So Jinja2 templates aren't actually python files. This would be better marked as a .txt file because Review Bot is going to keep having issues with it.

rbtools/utils/tien_template.py (Diff revision 1)

Show all issues

You are setting variables here but using review_request.bugs_closed later. Why not just use review_request.foo for everything?

tienv Feb. 4, 2015, 5 p.m.

For this default template, I took the old python print code and "translated" it directly into the template. Maybe it's for some readability reason? I can change it if it's important.

tienv March 2, 2015, 8:01 p.m.

I'm dropping this issue. I don't think the exact logic of the default template is too important, as it may change in the future (and by user defaults)

rbtools/utils/tien_template.py (Diff revision 1)

Show all issues

You don't want two blank lines between summary and description.

rbtools/utils/tien_template.py (Diff revision 1)

Show all issues

You don't want two blank lines between description and testing done.

You may want to put the blank line after the

{% if testing_done %}

bit.

rbtools/utils/tien_template.py (Diff revision 1)

Show all issues

Again, you probably don't want two blank lines here.

Branch:

-	tien_template
+	master

Commit:

-	d01821c33009976ea4a4b7e5fe99eed374ff14b0
+	01c0355dd1fe65eb3ecdf4ab429d015d7e849523

Diff:

Revision 2 (+81 -21)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/patch.py (Diff revision 2)
Show all issues
```
 'pdb' imported but unused
```

rbtools/commands/patch.py (Diff revision 2)

Show all issues

Col: 80
 E501 line too long (94 > 79 characters)

rbtools/commands/post.py (Diff revision 2)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/utils/commands.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/utils/commands.py (Diff revision 2)
Show all issues
```
Col: 66
 W291 trailing whitespace
```

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)
Show all issues
```
Col: 40
 W291 trailing whitespace
```

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 80
 E501 line too long (90 > 79 characters)

rbtools/utils/commands.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/utils/commands.py (Diff revision 2)
Show all issues
```
 undefined name 'default_templates_dir'
```

rbtools/utils/commands.py (Diff revision 2)

Show all issues

Col: 80
 E501 line too long (92 > 79 characters)

Commit:

-	01c0355dd1fe65eb3ecdf4ab429d015d7e849523
+	d9a9203c125d4614f8c040454d1832c209425e14

Diff:

Revision 3 (+78 -21)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

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

Show all issues

Col: 80
 E501 line too long (94 > 79 characters)

rbtools/commands/post.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/utils/commands.py (Diff revision 3)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/utils/commands.py (Diff revision 3)
Show all issues
```
Col: 66
 W291 trailing whitespace
```

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 3)
Show all issues
```
Col: 40
 W291 trailing whitespace
```

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

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

Show all issues

Col: 13
 E128 continuation line under-indented for visual indent

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

Show all issues

Col: 80
 E501 line too long (90 > 79 characters)

rbtools/utils/commands.py (Diff revision 3)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/utils/commands.py (Diff revision 3)
Show all issues
```
 undefined name 'default_templates_dir'
```

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

Show all issues

Col: 80
 E501 line too long (92 > 79 characters)

rbtools/templates/default.txt (Diff revision 3)

Show all issues

There's some inconsistency in the blank lines here. If the 'testing done' field is present, there's plenty of space, but if it's not, the bugs and review request URL will be jammed up right against the description.

We should also have a blank line after the summary.

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

Show all issues

os and pkg_resources are standard library modules, and so should be in their own group before six. six should be grouped with the jinja2 imports.

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

Show all issues

These should be a single import statement and kept alphabetical:

from jinja2 import (Environment, FileSystemLoader, StrictUndefined,
                    TemplateError, TemplateNotFound, UndefinedError)

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

Show all issues

extract_template is a pretty weird name for this. How about render_commit_message_template?

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

Show all issues

How about a message more like

raise CommandError('Syntax error in template %s: %s'
                   % (template_name, ue.message))

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

Show all issues

I'd like to see this error message contain the template name, and put it all on one line. Something like this:

raise CommandError('Unknown error while rendering commit-message template %s: %s' % (template_name, e.message))

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

Show all issues

This should probably check to make sure that the template name isn't absolute before joining it to cwd/templates

Commit:

-	d9a9203c125d4614f8c040454d1832c209425e14
+	c7585729d426a92df92c49b91d889a22977788a0

Diff:

Revision 4 (+80 -20)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/patch.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (94 > 79 characters)

rbtools/commands/post.py (Diff revision 4)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

rbtools/utils/commands.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

rbtools/utils/commands.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (99 > 79 characters)

rbtools/utils/commands.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (112 > 79 characters)

rbtools/utils/commands.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (84 > 79 characters)

rbtools/utils/commands.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (87 > 79 characters)

Commit:

-	c7585729d426a92df92c49b91d889a22977788a0
+	7be3d4f887ce09a819d7c1fc52c35f9e3985548d

Diff:

Revision 5 (+89 -24)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/patch.py (Diff revision 5)

Show all issues

Col: 80
 E501 line too long (94 > 79 characters)

rbtools/commands/post.py (Diff revision 5)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

rbtools/utils/commands.py (Diff revision 5)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

rbtools/utils/commands.py (Diff revision 5)

Show all issues

Col: 29
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 5)

Show all issues

Col: 29
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 5)
Show all issues
```
Col: 68
 W291 trailing whitespace
```

rbtools/utils/commands.py (Diff revision 5)

Show all issues

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

rbtools/utils/commands.py (Diff revision 5)
Show all issues
```
Col: 57
 W291 trailing whitespace
```

Please go through Review Bot's reviews and mark all of the issues that you've addressed as "Fixed"

Commit:

-	7be3d4f887ce09a819d7c1fc52c35f9e3985548d
+	37871855ae45b1dc81598e8116bb99bcded8c881

Diff:

Revision 6 (+110 -24)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/patch.py (Diff revision 6)

Show all issues

Col: 80
 E501 line too long (99 > 79 characters)

rbtools/commands/patch.py (Diff revision 6)
Show all issues
```
Col: 100
 W291 trailing whitespace
```

rbtools/commands/patch.py (Diff revision 6)

Show all issues

Col: 80
 E501 line too long (111 > 79 characters)

rbtools/commands/patch.py (Diff revision 6)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/commands/post.py (Diff revision 6)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

Col: 80
 E501 line too long (102 > 79 characters)

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

Show all issues

 local variable 'e' is assigned to but never used

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

Show all issues

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

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

Show all issues

Col: 80
 E501 line too long (82 > 79 characters)

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

Show all issues

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

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

Show all issues

Col: 80
 E501 line too long (83 > 79 characters)

rbtools/utils/commands.py (Diff revision 6)
Show all issues
```
Col: 42
 W291 trailing whitespace
```

Groups:

+	students

Commit:

-	37871855ae45b1dc81598e8116bb99bcded8c881
+	453c64b435ae5182e110277889435e396251f955

Diff:

Revision 7 (+118 -24)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/utils/commands.py (Diff revision 7)

Show all issues

 local variable 'e' is assigned to but never used

Commit:

-	453c64b435ae5182e110277889435e396251f955
+	ea8f48f6520bd6e28ef942ef877d7f401b1ab0a0

Diff:

Revision 8 (+122 -24)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/patch.py (Diff revision 8)

Show all issues

Col: 80
 E501 line too long (84 > 79 characters)

rbtools/commands/patch.py (Diff revision 8)

Show all issues

Col: 80
 E501 line too long (98 > 79 characters)

rbtools/commands/post.py (Diff revision 8)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

rbtools/utils/commands.py (Diff revision 8)

Show all issues

Col: 31
 E128 continuation line under-indented for visual indent

rbtools/utils/commands.py (Diff revision 8)

Show all issues

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

rbtools/utils/commands.py (Diff revision 8)
Show all issues
```
Col: 59
 W291 trailing whitespace
```

rbtools/utils/commands.py (Diff revision 8)

Show all issues

Col: 80
 E501 line too long (89 > 79 characters)

rbtools/utils/commands.py (Diff revision 8)
Show all issues
```
Col: 71
 W291 trailing whitespace
```

rbtools/commands/patch.py (Diff revision 8)

Show all issues

self.config.get(name) should be the equivalent of the self.config[name] if condition else None

rbtools/utils/commands.py (Diff revision 8)

Show all issues

I noticed you have three cases. For the case where there's only template_name, it get combines with working directory and template_name becomes empty. For the case where there's a template_dir, it gets added to working directory, and the template_name can or can not be empty. It seems a bit inconsistent. Not familiar with the templating, so I don't know if it accomodates for that, but my view I feel it may cause errors.

tienv Feb. 2, 2015, 8:19 p.m.

I'm not sure about this logic either. The template has to be somewhere
a. if it's in the .reviewboardrc file (DEFAULT_TEMPLATE), just use that
b. there can be a default template folder in .reviewboardrc (TEMPLATES).
now rbt patch --commit --template <template_name> can look in the default template folder for <template_name>
c. if there is no default template folder defined, I thought it would be useful to do this
rbt patch --commit --template <template_name> (where template_name is the directory)
d. nothing is defined
rbt patch --commit

just uses the default template

is there a better way to do this?

VTL-Developer Feb. 3, 2015, 6:24 a.m.

Sorry about one part. The color confused me a bit. When there's a template_dir, it needs to have a template_name to proceed forward. When there's template_name, that gets removed changed to an empty string and template_dir gets working directory and the name of the file.

The first case seems okay, for the second second, you can keep the template_name the same, and change template_dir to working directory.

tienv Feb. 3, 2015, 8:07 p.m.

yeah this doesn't make a lot of sense, i'll change this

Commit:

-	ea8f48f6520bd6e28ef942ef877d7f401b1ab0a0
+	ca5b00d8ec7391d51d2c9fc4c0586df40bf238a4

Diff:

Revision 9 (+119 -24)

Show changes

	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/utils/commands.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/post.py (Diff revision 9)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

rbtools/commands/patch.py (Diff revision 9)
Show all issues
```
Add a real help description.
```
rbtools/templates/default.txt (Diff revision 9)
Show all issues
```
One blank line instead of two.
```
rbtools/templates/default.txt (Diff revision 9)
Show all issues
```
Add a blank line between these two.
```
rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
Undo this change.
```

rbtools/utils/commands.py (Diff revision 9)

Show all issues

We only use trailing commas on the last line of a dictionary definition.

rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
Move this up to the previous line.
```

rbtools/utils/commands.py (Diff revision 9)

Show all issues

Use e instead of tnfe. This is for consistency with the rest of the codebase.

rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
Use e instead of ue.
```
rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
Use e instead of te.
```

rbtools/utils/commands.py (Diff revision 9)

Show all issues

We don't use str.format. Please use % instead for string formatting.

rbtools/utils/commands.py (Diff revision 9)

Show all issues

More than one of these can fit on a line.

rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
Blank line between a statement and a block
```

rbtools/utils/commands.py (Diff revision 9)

Show all issues

We don't use str.format. Please use % for formatting instead.

rbtools/utils/commands.py (Diff revision 9)

Show all issues

This string isn't formatted correctly. There will be no space between file and must. Also, please use all available line width efficiently.

rbtools/utils/commands.py (Diff revision 9)

Show all issues

Since you are doing the following:

else:
    if foo:
        bar()
    else:
        baz()


This can be changed to:

elif foo:
    bar()
else:
    baz()


This reduces the nesting and makes the code easier to follow.

rbtools/utils/commands.py (Diff revision 9)

Show all issues

Isn't this the template filename, not the directory?

rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
One blank line between these.
```
rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
Why unset the name here?
```
rbtools/utils/commands.py (Diff revision 9)
Show all issues
```
Use %s instead for string formatting.
```

rbtools/utils/commands.py (Diff revision 9)

Show all issues

More than one of these can fit on a line.

Commit:

-	ca5b00d8ec7391d51d2c9fc4c0586df40bf238a4
+	e951fdfe386b25e65bc365f5a3c89ced18f240f2

Diff:

Revision 10 (+135 -25)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/post.py (Diff revision 10)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/utils/commands.py (Diff revision 10)
Show all issues
```
 'Template' imported but unused
```

rbtools/utils/commands.py (Diff revision 10)

Show all issues

Col: 80
 E501 line too long (89 > 79 characters)

rbtools/utils/commands.py (Diff revision 10)

Show all issues

Col: 80
 E501 line too long (83 > 79 characters)

rbtools/utils/commands.py (Diff revision 10)

Show all issues

Col: 80
 E501 line too long (80 > 79 characters)

Commit:

-	e951fdfe386b25e65bc365f5a3c89ced18f240f2
+	3f6eb04aaeaadcac44c2da017563fd90c86fe38d

Diff:

Revision 11 (+135 -25)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/post.py (Diff revision 11)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/utils/commands.py (Diff revision 11)
Show all issues
```
 undefined name 'ue'
```

Summary:

-	Add commit message template prototype
+	Add commit message template

Description:

~		This is a draft only
	~	Some RBT commands, like patch/land, allow users to commit with a commit message. Previously, this commit message would be hard coded with python strings, however adding the option to specify a user defined template would be more helpful.
	+
	+	The added option is --template <template_directory_or_name>, which could be any of:
	+	- the template's directory
	+	- the template's directory within the "TEMPLATES_DIR" folder, as specified in .reviewboardrc
	+	- the name of the template, as specified in .reviewboardrc
	+
	+	If no template is specified, a default template is selected.
	+	If no template exists under the argument, the user will be prompted for the default template.
	+	As template design is up to the user, the code is not responsible for handling misconfigured templates.

Testing Done:

	+	I've tested with (rbt patch):
	+	--template <existing_template_path>
	+	--template <non_existant_template_path>
	+	--template <existing_template in TEMPLATES_DIR>
	+	--template <variable name in .reviewboardrc>
	+	--template with invalid syntax (like not closing the if statement)
	+	--template attempting to access invalid variables
	+	--commit (no --template, which should render the default template)
	+
	+	Unit tests are available as well, but I don't know if they make sense tested that way.

Commit:

-	3f6eb04aaeaadcac44c2da017563fd90c86fe38d
+	265f134d4fcbfffcbd4e5ea82acb75307e20f250

Diff:

Revision 12 (+239 -24)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/post.py (Diff revision 12)
Show all issues
```
Please revert this change.
```
rbtools/commands/post.py (Diff revision 12)
Show all issues
```
Please revert this change.
```

rbtools/commands/post.py (Diff revision 12)

Show all issues

This is part of a separate review request and should not be present in this change.

rbtools/utils/commands.py (Diff revision 12)

Show all issues

How about just doing import jinja2 and then using qualified names (jinja2.Environment) for each of these? Many of these names are pretty generic, and when looking at names like Environment and UndefinedError it's not immediately apparent that these are part of jinja2.

rbtools/utils/commands.py (Diff revision 12)

Show all issues

This should be a fully-qualified module name (rbtools.utils.console)

rbtools/utils/commands.py (Diff revision 12)

Show all issues

This method should have a docstring.

TI

tienv Feb. 25, 2015, 10:32 p.m.
```
Doesn't it already have one?
```

david Feb. 26, 2015, 1:44 p.m.

I don't see one, either in this revision or your latest revision.

rbtools/utils/commands.py (Diff revision 12)

Show all issues

Based on this code, it looks like directory is actually a filename, not a directory.

tienv Feb. 25, 2015, 10:32 p.m.

ah, so filename is the whole thing. now I understand the difference

rbtools/utils/commands.py (Diff revision 12)

Show all issues

Comments should have proper capitalization and punctuation.

rbtools/utils/commands.py (Diff revision 12)

Show all issues

We generally don't use try/else. Instead, you can move template.render() into the try case above and then have all the exception handlers at one level:

try:
    env = Environment(...)
    template = env.get_template(template_name)
    return template.render(review_request=review_request)
except TemplateNotFound as e:
    ...
except UndefinedError as e:
    ...
...

TI

tienv Feb. 25, 2015, 10:32 p.m.
```
ok. is this a preference?
```
david Feb. 26, 2015, 1:44 p.m.
```
Yes.
```

rbtools/utils/commands.py (Diff revision 12)

Show all issues

Is this the best name for this method? Probably not.

rbtools/utils/commands.py (Diff revision 12)

Show all issues

This doesn't look like it's returning the directory name (rather, it looks like it's returning a filename).

The explanation of what this method does should also be a docstring instead of a comment.

rbtools/utils/commands.py (Diff revision 12)

Show all issues

This function isn't terribly well named anymore. Can we change it to be build_commit_message?

Change Summary:

Changed mostly formatting issues based on David's code reviews.

I am pretty sure this works, what do I have to do next to land this?

Commit:

-	265f134d4fcbfffcbd4e5ea82acb75307e20f250
+	60fb63308b25b0659198d233fc995297b9c64393

Diff:

Revision 13 (+233 -24)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/land.py (Diff revision 13)
Show all issues
```
 'extract_commit_message' imported but unused
```
rbtools/commands/land.py (Diff revision 13)
Show all issues
```
 undefined name 'build_commit_message'
```
rbtools/commands/patch.py (Diff revision 13)
Show all issues
```
 'extract_commit_message' imported but unused
```
rbtools/commands/patch.py (Diff revision 13)
Show all issues
```
 undefined name 'build_commit_message'
```

Commit:

-	60fb63308b25b0659198d233fc995297b9c64393
+	0983e00055a0a7f44c932e143f37b09735c0977f

Diff:

Revision 14 (+235 -26)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/commands/post.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/commands/post.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/post.py (Diff revision 14)
Show all issues
```
Please revert this change.
```
rbtools/commands/post.py (Diff revision 14)
Show all issues
```
Revert this change.
```

rbtools/templates/default.txt (Diff revision 14)

Show all issues

I'm not sure why you set these variables, given that later on you use review_request.bugs_closed. Can we just prefix all variables with review_request.?

rbtools/templates/default.txt (Diff revision 14)
Show all issues
```
Should have a space before the closing %}
```
rbtools/templates/default.txt (Diff revision 14)
Show all issues
```
Should have a space before the closing %}
```

rbtools/utils/commands.py (Diff revision 14)

Show all issues

This should be a docstring, not a comment (and change 'Returns' to 'Return' and add a period at the end.)

rbtools/utils/commands.py (Diff revision 14)
Show all issues
```
Returns -> Return.
```

rbtools/utils/commands.py (Diff revision 14)

Show all issues

What is 'template_cmnd'? That string doesn't appear anywhere else in this change.

rbtools/utils/commands.py (Diff revision 14)

Show all issues

This should present an error that the commit message template directory could not be found.

rbtools/utils/tests.py (Diff revision 14)
Show all issues
```
Please add a class docstring.
```
rbtools/utils/tests.py (Diff revision 14)
Show all issues
```
Indent this 4 more spaces.
```

rbtools/utils/tests.py (Diff revision 14)

Show all issues

Please add docstrings for all the test methods in here. We want to print nice things with nosetests -v

Commit:

-	0983e00055a0a7f44c932e143f37b09735c0977f
+	1a90955575aa5ba6d2e2ef13524e4a6e2e14e6f4

Diff:

Revision 15 (+246 -26)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/land.py (Diff revision 15)
Show all issues
```
Alphabetize these.
```

rbtools/commands/land.py (Diff revision 15)

Show all issues

I would reword this to say something like

"Use the specified template (either in the directory specified in .reviewboardrc or by its path) to generate the commit message."

Also, I would put the space on the previous line because it reads more nicely.

rbtools/commands/land.py (Diff revision 15)
Show all issues
```
We use single quotes for strings.
```
rbtools/commands/land.py (Diff revision 15)
Show all issues
```
This should be on the previous line.
```

rbtools/commands/patch.py (Diff revision 15)

Show all issues

Since we're using this in multiple places, this might be worth putting into a list in __init__.py and referencing it elsewhere (so that we're not repeating ourselves).

tienv March 16, 2015, 7:32 p.m.

Makes sense. The pattern I see now is doing Command.some_option. I guess I can make an OptionGroup with a single template option in Command and reference it from both patch and land?

rbtools/commands/patch.py (Diff revision 15)
Show all issues
```
See my other comment.
```
rbtools/commands/patch.py (Diff revision 15)
Show all issues
```
We use single quotes for strings.
```
rbtools/commands/patch.py (Diff revision 15)
Show all issues
```
This should be on the previous line.
```

rbtools/utils/commands.py (Diff revision 15)

Show all issues

Use single quotes for all these strings.

rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Can you put the first %s in double quotes?
```

rbtools/utils/commands.py (Diff revision 15)

Show all issues

Use a %-formatting expression. Its more efficient than string concatenation.

rbtools/utils/commands.py (Diff revision 15)

Show all issues

Can you format that is as a reStructured Text List and mark the keys with double backticks, e.g.

 * ``foo``: bar
 * ``bar``: baz

rbtools/utils/commands.py (Diff revision 15)

Show all issues

Can we rename template_cmd to something like template_arg or specified_template? template_cmd seems like an inaccurate name for it.

TI

tienv March 16, 2015, 7:32 p.m.
```
yeah template_arg makes sense
```

rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Maybe this should this just be template ?
```
rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Blank line between these.
```
rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Put this on the previous line.
```
rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Use a %-formatting expression here.
```
rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Blank line between these.
```
rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Blank line between these.
```
rbtools/utils/commands.py (Diff revision 15)
Show all issues
```
Blank line between these.
```
rbtools/utils/tests.py (Diff revision 15)
Show all issues
```
Alphabetize these.
```

Commit:

-	1a90955575aa5ba6d2e2ef13524e4a6e2e14e6f4
+	99c4feed15dd65599afda282355327b1be8c7314

Diff:

Revision 16 (+250 -27)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/utils/commands.py (Diff revision 16)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

Commit:

-	99c4feed15dd65599afda282355327b1be8c7314
+	949c5dd75510fe2413cadcf4ca053c96cc3459f3

Diff:

Revision 17 (+250 -27)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/utils/commands.py (Diff revision 17)

Show all issues

Col: 80
 E501 line too long (81 > 79 characters)

```
Just some style nits:
```

rbtools/utils/commands.py (Diff revision 17)

Show all issues

Can you format the docstring as such:

def render_template(filename, review_request):
   """Render a commit message template.

   It is up to the template author to ensure that the syntax
   is correct. If not, this method will raise an error with
   Jinja2's error message.
   """   


Specifically, the changes are:

- First line should be a short summary.

- Blank line between summary and more extensive description.

- Imperative mood in the summary ("Render" rather than "Renders").

- Periods at the end of sentences.

rbtools/utils/commands.py (Diff revision 17)
Show all issues
```
This should be a docstring, not a comment.
```

rbtools/utils/commands.py (Diff revision 17)

Show all issues

No blank line between the docstring and code for methods.

rbtools/utils/tests.py (Diff revision 17)

Show all issues

All test docstrings should start with "Testing" instead of "Test". Here and below.

Commit:

-	949c5dd75510fe2413cadcf4ca053c96cc3459f3
+	8afca6cac19c50b5f64a5d0c2bef412519a65cb6

Diff:

Revision 18 (+248 -23)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

Diff:

Revision 19 (+248 -23)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/utils/commands.py (Diff revision 19)

Show all issues

First line of a docstring should come right after """ ("""Render a template...)

rbtools/utils/commands.py (Diff revision 19)

Show all issues

This should just be """Return the filename of the default template.""" (note: one line, with period)

rbtools/utils/tests.py (Diff revision 19)

Show all issues

Instead of defining a new empty class for this, I think you could just do mock_review_request = object()

tienv March 20, 2015, 11:03 p.m.

I can't do that, as I wanted to place an attribute in the object

mock_review_request.val = 'some_value'

Commit:

-	8afca6cac19c50b5f64a5d0c2bef412519a65cb6
+	e3ac4dd03f0805b3af95fc67e3ef4092e69c5d51

Diff:

Revision 20 (+249 -27)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/utils/commands.py (Diff revision 20)
Show all issues
```
alphabetical order
```

Commit:

-	e3ac4dd03f0805b3af95fc67e3ef4092e69c5d51
+	52f0ddd7bfd2f43f00b2c7f9c2c0ce89be59054d

Diff:

Revision 21 (+249 -27)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/land.py (Diff revision 21)

Show all issues

If .reviewboardrc specifies a template, it should be used for all commits, without needing to pass an argument.

tienv April 1, 2015, 5:28 p.m.

in patch/land, I have it pass this in instead

            template_name_or_dir = self.options.custom_template or 'DEFAULT_TEMPLATE'

extract_commit_message can look up whatever this happens to be in .reviewboardrc (could be something besides 'DEFAULT_TEMPLATE')

I think this works, but if this is weird then I can fix it

chipx86 April 1, 2015, 8:47 p.m.

I think, from a user's point of view, when looking at the options, that it's a bit confusing.

What I'd expect to happen is that --template would specify the template filename to use for the commit, overriding either what's in .reviewboardrc or what's used by default in RBTools, respectively.

In that case, you can also benefit from having this take a config_key, meaning that self.options.custom_template will always either be the value specified on the command line or it'll be the value in .reviewboardrc (if set).

rbtools/commands/land.py (Diff revision 21)
Show all issues
```
This isn't needed.
```

rbtools/commands/land.py (Diff revision 21)

Show all issues

The wrapping should use up more of the line.

rbtools/commands/patch.py (Diff revision 21)
Show all issues
```
Same comments here.
```
rbtools/utils/commands.py (Diff revision 21)
Show all issues
```
This should be part of the next group of imports.
```

rbtools/utils/commands.py (Diff revision 21)

Show all issues

This should use format strings, instead of string concatenation.

rbtools/utils/commands.py (Diff revision 21)
Show all issues
```
Blank line between these.
```

rbtools/utils/commands.py (Diff revision 21)

Show all issues

This is deprecated. You'll need to rewrite the code to use os.walk instead.

rbtools/utils/commands.py (Diff revision 21)

Show all issues

Looks like trim_blocks will easily fit on the previous line.

rbtools/utils/commands.py (Diff revision 21)

Show all issues

I don't know that a newline is appropriate here. Can it just be: 'Template error: %s' ?

rbtools/utils/commands.py (Diff revision 21)

Show all issues

You can use os.path.abspath(template_arg) instead, and it will do the equivalent.

rbtools/utils/commands.py (Diff revision 21)
Show all issues
```
This leaves out the space between sentences.
```

rbtools/utils/commands.py (Diff revision 21)

Show all issues

This code is repeating itself. You should rewrite it to only do this once.

rbtools/utils/commands.py (Diff revision 21)
Show all issues
```
Same here.
```
rbtools/utils/commands.py (Diff revision 21)
Show all issues
```
Missing a trailing period.
```
rbtools/utils/commands.py (Diff revision 21)
Show all issues
```
Missing a trailing period.
```
rbtools/utils/tests.py (Diff revision 21)
Show all issues
```
Must be in alphabetical order.
```

rbtools/utils/tests.py (Diff revision 21)

Show all issues

You shouldn't really use """ here. Instead, it's best to just do:

mock_template = (
    'Start\n'
    '...\n'
    '...\n')

That way, we better control newline characters and indentation, producing more reliable test results.

Same with all others.

rbtools/utils/tests.py (Diff revision 21)

Show all issues

These really should go in the tests themselves, so that they're self-contained and not vulnerable to impact from another test wanting to modify a string slightly.

tienv April 1, 2015, 5:28 p.m.

I changed the variable names to resemble Python "constants". These should not be altered

chipx86 April 1, 2015, 8:47 p.m.

I'd still like them to be in each test, even if they're repeated.

The reason is that a test should ideally be self-contained and easy to read without looking up other things. They should also be free of side-effects from other tests.

By sharing these strings, I, as a reviewer and later as someone who maintains this code, will need to read the test body, then read the strings, then jump back, then up for the results, then back to the test code, in order to have an idea of what content is being tested against.

Later, if I want to add a test with content that's a slight variation, I have to decide whether to bring this in to my test case, or whether it should be a global string. It's best just to follow the pattern other tests have (see our diff tests, for instance) and bring the content into the test functions.

rbtools/utils/tests.py (Diff revision 21)
Show all issues
```
This should be done in setUp().
```

Commit:

-	52f0ddd7bfd2f43f00b2c7f9c2c0ce89be59054d
+	98968a8cf6e4dbb851aaced919c148041eb21c66

Diff:

Revision 22 (+230 -27)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/land.py (Diff revision 22)

Show all issues

I missed this part of the design before. I realize now that this is allowing multiple templates per tree. I've discussed this with others, and I think it's probably best to keep this simple and just have a single template specified in .reviewboardc.

I think we should focus on one template, and don't provide an option to override.

The reason is that a project is going to have a standard, and they want all commits to meet that standard. If something more custom is needed, it can just be a manually-created/augmented commit, but I don't think there are any cases where people are going to need multiple templates here.

What use-cases did you have in mind?

tienv April 1, 2015, 9:41 p.m.

I was thinking somebody could put:
TEMPLATE_1 = something
TEMPLATE_2 = something_else

in .reviewboardrc

and then do --template TEMPLATE_1 or --template TEMPLATE_2

Alternatively, they can specify
TEMPLATES_DIR = 'templates/'

with templates containing:
template_1.txt
template_2.txt

where either --template template_1.txt or --template template_2.txt can work

is this overdoing it? I understand what you are saying, that organizations might just stick to one standard template. This works too. I'm not sure. Changing it either way shouldn't be too hard

rbtools/utils/commands.py (Diff revision 22)

Show all issues

No backticks here. It'll actually render incorrectly, as comments are meant to be in ReST syntax, where backticks are used for document references.

rbtools/utils/commands.py (Diff revision 22)

Show all issues

Might be nice to have a bit of information on the format of the template here. Things like how variables/functions are used.

rbtools/utils/commands.py (Diff revision 22)

Show all issues

You should be able to use os.path.split here, to do this in one go.

tienv April 2, 2015, 4:31 p.m.

I think it still needs 2 lines though, as I need both of the variables later

rbtools/utils/commands.py (Diff revision 22)
Show all issues
```
Missing a trailing period.
```
rbtools/utils/commands.py (Diff revision 22)
Show all issues
```
Should use single quotes where possible.
```
rbtools/utils/commands.py (Diff revision 22)
Show all issues
```
"Error" shouldn't be capitalized.
```

rbtools/utils/commands.py (Diff revision 22)

Show all issues

We may want to check that this is a directory first, and provide an error otherwise.

rbtools/utils/commands.py (Diff revision 22)

Show all issues

This should just be:

if not default_templates:

Better than checking < 1, since the only reasonable value is 0.

rbtools/utils/commands.py (Diff revision 22)
Show all issues
```
Blank line here.
```

rbtools/utils/commands.py (Diff revision 22)

Show all issues

I kind of think that if the template is missing, we should just error out. That's a very clear configuration problem.

TI

tienv April 1, 2015, 9:51 p.m.
```
what do you mean by "erroring it out"?
```

rbtools/utils/commands.py (Diff revision 22)

Show all issues

Missing a trailing period.

"Raises" is maybe the wrong word here?

Commit:

-	98968a8cf6e4dbb851aaced919c148041eb21c66
+	f16901700e1ff23fa5b30fcf8b76edbb778d5c88

Diff:

Revision 23 (+248 -27)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

Diff:

Revision 24 (+239 -26)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/land.py (Diff revision 24)
Show all issues
```
 'build_commit_message' imported but unused
```
rbtools/commands/land.py (Diff revision 24)
Show all issues
```
Col: 69
 W291 trailing whitespace
```
rbtools/commands/land.py (Diff revision 24)
Show all issues
```
 undefined name 'extract_commit_message'
```

Diff:

Revision 25 (+239 -26)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

rbtools/commands/land.py (Diff revision 25)
Show all issues
```
 'build_commit_message' imported but unused
```
rbtools/commands/land.py (Diff revision 25)
Show all issues
```
Col: 69
 W291 trailing whitespace
```
rbtools/commands/land.py (Diff revision 25)
Show all issues
```
 undefined name 'extract_commit_message'
```

Diff:

Revision 26 (+240 -27)

Show changes

	rbtools/commands/land.py
	rbtools/commands/patch.py
	rbtools/templates/default.txt
	rbtools/utils/commands.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py
    rbtools/utils/commands.py
    rbtools/utils/tests.py
    rbtools/commands/patch.py

Ignored Files:
    rbtools/templates/default.txt

Add commit message template

Branch:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Groups:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Summary:

Description:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Diff:

Diff:

Diff:

Status: Discarded