Add commit message template

Review Request #6833 — Created Jan. 27, 2015 and discarded

Information

RBTools
master

Reviewers

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.

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.

Description From Last Updated

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

This doesn't belong in this change.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Col: 62 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 40 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Usually the pattern is except TemplateNotFound as e: Same below.

brenniebrennie

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

brenniebrennie

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

So Jinja2 templates aren't actually python files. This would be better marked as a .txt file because Review Bot is …

brenniebrennie

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 42 E225 missing whitespace around operator

reviewbotreviewbot

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

brenniebrennie

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 50 E225 missing whitespace around operator

reviewbotreviewbot

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 52 E225 missing whitespace around operator

reviewbotreviewbot

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 44 E225 missing whitespace around operator

reviewbotreviewbot

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

brenniebrennie

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 11 E225 missing whitespace around operator

reviewbotreviewbot

You don't want two blank lines between description and testing done. You may want to put the blank line after …

brenniebrennie

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 19 E228 missing whitespace around modulo operator

reviewbotreviewbot

Col: 1 E112 expected an indented block

reviewbotreviewbot

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 11 E225 missing whitespace around operator

reviewbotreviewbot

Col: 33 E228 missing whitespace around modulo operator

reviewbotreviewbot

Col: 2 E225 missing whitespace around operator

reviewbotreviewbot

Col: 1 E112 expected an indented block

reviewbotreviewbot

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

brenniebrennie

'pdb' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 66 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 40 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

undefined name 'default_templates_dir'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

There's some inconsistency in the blank lines here. If the 'testing done' field is present, there's plenty of space, but …

daviddavid

os and pkg_resources are standard library modules, and so should be in their own group before six. six should be …

daviddavid

These should be a single import statement and kept alphabetical: from jinja2 import (Environment, FileSystemLoader, StrictUndefined, TemplateError, TemplateNotFound, UndefinedError)

daviddavid

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

daviddavid

Col: 66 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 40 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

How about a message more like raise CommandError('Syntax error in template %s: %s' % (template_name, ue.message))

daviddavid

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

daviddavid

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

daviddavid

undefined name 'default_templates_dir'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 68 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 57 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 100 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'e' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

local variable 'e' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

VT VTL-Developer

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

Col: 59 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 71 W291 trailing whitespace

reviewbotreviewbot

Add a real help description.

brenniebrennie

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

One blank line instead of two.

brenniebrennie

Add a blank line between these two.

brenniebrennie

Undo this change.

brenniebrennie

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

brenniebrennie

Move this up to the previous line.

brenniebrennie

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

brenniebrennie

Use e instead of ue.

brenniebrennie

Use e instead of te.

brenniebrennie

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

brenniebrennie

More than one of these can fit on a line.

brenniebrennie

Blank line between a statement and a block

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Since you are doing the following: else: if foo: bar() else: baz() This can be changed to: elif foo: bar() …

brenniebrennie

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

brenniebrennie

One blank line between these.

brenniebrennie

Why unset the name here?

brenniebrennie

Use %s instead for string formatting.

brenniebrennie

More than one of these can fit on a line.

brenniebrennie

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

'Template' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

undefined name 'ue'

reviewbotreviewbot

Please revert this change.

daviddavid

Please revert this change.

daviddavid

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

daviddavid

How about just doing import jinja2 and then using qualified names (jinja2.Environment) for each of these? Many of these names …

daviddavid

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

daviddavid

This method should have a docstring.

daviddavid

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

daviddavid

Comments should have proper capitalization and punctuation.

daviddavid

We generally don't use try/else. Instead, you can move template.render() into the try case above and then have all the …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

'extract_commit_message' imported but unused

reviewbotreviewbot

undefined name 'build_commit_message'

reviewbotreviewbot

'extract_commit_message' imported but unused

reviewbotreviewbot

undefined name 'build_commit_message'

reviewbotreviewbot

Please revert this change.

daviddavid

Revert this change.

daviddavid

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

daviddavid

Should have a space before the closing %}

daviddavid

Should have a space before the closing %}

daviddavid

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

daviddavid

Returns -> Return.

daviddavid

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

daviddavid

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

daviddavid

Please add a class docstring.

daviddavid

Indent this 4 more spaces.

daviddavid

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

daviddavid

Alphabetize these.

brenniebrennie

I would reword this to say something like "Use the specified template (either in the directory specified in .reviewboardrc or …

brenniebrennie

We use single quotes for strings.

brenniebrennie

This should be on the previous line.

brenniebrennie

Since we're using this in multiple places, this might be worth putting into a list in __init__.py and referencing it …

brenniebrennie

See my other comment.

brenniebrennie

We use single quotes for strings.

brenniebrennie

This should be on the previous line.

brenniebrennie

Use single quotes for all these strings.

brenniebrennie

Can you put the first %s in double quotes?

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Maybe this should this just be template ?

brenniebrennie

Blank line between these.

brenniebrennie

Put this on the previous line.

brenniebrennie

Use a %-formatting expression here.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Alphabetize these.

brenniebrennie

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

reviewbotreviewbot

Can you format the docstring as such: def render_template(filename, review_request): """Render a commit message template. It is up to the …

daviddavid

This should be a docstring, not a comment.

daviddavid

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

reviewbotreviewbot

No blank line between the docstring and code for methods.

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

alphabetical order

VT VTL-Developer

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

chipx86chipx86

This isn't needed.

chipx86chipx86

The wrapping should use up more of the line.

chipx86chipx86

Same comments here.

chipx86chipx86

This should be part of the next group of imports.

chipx86chipx86

This should use format strings, instead of string concatenation.

chipx86chipx86

Blank line between these.

chipx86chipx86

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

chipx86chipx86

Looks like trim_blocks will easily fit on the previous line.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This leaves out the space between sentences.

chipx86chipx86

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

chipx86chipx86

Same here.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Must be in alphabetical order.

chipx86chipx86

You shouldn't really use """ here. Instead, it's best to just do: mock_template = ( 'Start\n' '...\n' '...\n') That way, …

chipx86chipx86

These really should go in the tests themselves, so that they're self-contained and not vulnerable to impact from another test …

chipx86chipx86

This should be done in setUp().

chipx86chipx86

I missed this part of the design before. I realize now that this is allowing multiple templates per tree. I've …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Should use single quotes where possible.

chipx86chipx86

"Error" shouldn't be capitalized.

chipx86chipx86

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

chipx86chipx86

This should just be: if not default_templates: Better than checking < 1, since the only reasonable value is 0.

chipx86chipx86

Blank line here.

chipx86chipx86

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

chipx86chipx86

Missing a trailing period. "Raises" is maybe the wrong word here?

chipx86chipx86

'build_commit_message' imported but unused

reviewbotreviewbot

Col: 69 W291 trailing whitespace

reviewbotreviewbot

undefined name 'extract_commit_message'

reviewbotreviewbot

'build_commit_message' imported but unused

reviewbotreviewbot

Col: 69 W291 trailing whitespace

reviewbotreviewbot

undefined name 'extract_commit_message'

reviewbotreviewbot
reviewbot
  1. 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.
  2. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 62
     W291 trailing whitespace
    
  4. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  5. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 40
     W291 trailing whitespace
    
  6. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  7. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  8. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  9. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  10. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  11. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  12. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  13. rbtools/utils/commands.py (Diff revision 1)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  14. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  15. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 42
     E225 missing whitespace around operator
    
  16. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  17. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 50
     E225 missing whitespace around operator
    
  18. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  19. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 52
     E225 missing whitespace around operator
    
  20. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  21. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 44
     E225 missing whitespace around operator
    
  22. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  23. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 11
     E225 missing whitespace around operator
    
  24. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  25. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 19
     E228 missing whitespace around modulo operator
    
  26. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 1
     E112 expected an indented block
    
  27. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  28. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 11
     E225 missing whitespace around operator
    
  29. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 33
     E228 missing whitespace around modulo operator
    
  30. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 2
     E225 missing whitespace around operator
    
  31. rbtools/utils/tien_template.py (Diff revision 1)
     
     
    Col: 1
     E112 expected an indented block
    
  32. 
      
brennie
  1. 
      
  2. rbtools/commands/post.py (Diff revision 1)
     
     
     
     

    This doesn't belong in this change.

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

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

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

    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.

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

    Usually the pattern is

    except TemplateNotFound as e:
    

    Same below.

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

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

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

    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.

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

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

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

    2. 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)

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

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

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

    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.

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

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

  12. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/patch.py (Diff revision 2)
     
     
     'pdb' imported but unused
    
  3. rbtools/commands/patch.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  4. rbtools/commands/post.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  5. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  6. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 66
     W291 trailing whitespace
    
  7. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  8. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 40
     W291 trailing whitespace
    
  9. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  10. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  11. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  12. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  13. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  14. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  15. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  16. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (90 > 79 characters)
    
  17. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  18. rbtools/utils/commands.py (Diff revision 2)
     
     
     undefined name 'default_templates_dir'
    
  19. rbtools/utils/commands.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (92 > 79 characters)
    
  20. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/patch.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  3. rbtools/commands/post.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 66
     W291 trailing whitespace
    
  6. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  7. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 40
     W291 trailing whitespace
    
  8. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  9. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  10. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  11. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  12. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  13. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  14. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  15. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (90 > 79 characters)
    
  16. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  17. rbtools/utils/commands.py (Diff revision 3)
     
     
     undefined name 'default_templates_dir'
    
  18. rbtools/utils/commands.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (92 > 79 characters)
    
  19. 
      
david
  1. 
      
  2. rbtools/templates/default.txt (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

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

    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.

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

    These should be a single import statement and kept alphabetical:

    from jinja2 import (Environment, FileSystemLoader, StrictUndefined,
                        TemplateError, TemplateNotFound, UndefinedError)
    
  5. rbtools/utils/commands.py (Diff revision 3)
     
     

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

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

    How about a message more like

    raise CommandError('Syntax error in template %s: %s'
                       % (template_name, ue.message))
    
  7. rbtools/utils/commands.py (Diff revision 3)
     
     

    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))
    
  8. rbtools/utils/commands.py (Diff revision 3)
     
     
     

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

  9. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/patch.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  3. rbtools/commands/post.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. rbtools/utils/commands.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. rbtools/utils/commands.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (99 > 79 characters)
    
  6. rbtools/utils/commands.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (112 > 79 characters)
    
  7. rbtools/utils/commands.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  8. rbtools/utils/commands.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  9. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/patch.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  3. rbtools/commands/post.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. rbtools/utils/commands.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. rbtools/utils/commands.py (Diff revision 5)
     
     
    Col: 29
     E128 continuation line under-indented for visual indent
    
  6. rbtools/utils/commands.py (Diff revision 5)
     
     
    Col: 29
     E128 continuation line under-indented for visual indent
    
  7. rbtools/utils/commands.py (Diff revision 5)
     
     
    Col: 68
     W291 trailing whitespace
    
  8. rbtools/utils/commands.py (Diff revision 5)
     
     
    Col: 37
     E127 continuation line over-indented for visual indent
    
  9. rbtools/utils/commands.py (Diff revision 5)
     
     
    Col: 57
     W291 trailing whitespace
    
  10. 
      
david
  1. Please go through Review Bot's reviews and mark all of the issues that you've addressed as "Fixed"

  2. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/patch.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (99 > 79 characters)
    
  3. rbtools/commands/patch.py (Diff revision 6)
     
     
    Col: 100
     W291 trailing whitespace
    
  4. rbtools/commands/patch.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (111 > 79 characters)
    
  5. rbtools/commands/patch.py (Diff revision 6)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. rbtools/commands/post.py (Diff revision 6)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 33
     E127 continuation line over-indented for visual indent
    
  8. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 33
     E127 continuation line over-indented for visual indent
    
  9. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (102 > 79 characters)
    
  10. rbtools/utils/commands.py (Diff revision 6)
     
     
     local variable 'e' is assigned to but never used
    
  11. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 37
     E127 continuation line over-indented for visual indent
    
  12. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  13. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 33
     E127 continuation line over-indented for visual indent
    
  14. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  15. rbtools/utils/commands.py (Diff revision 6)
     
     
    Col: 42
     W291 trailing whitespace
    
  16. 
      
TI
TI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/patch.py
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    Ignored Files:
        rbtools/templates/default.txt
    
    
  2. rbtools/utils/commands.py (Diff revision 7)
     
     
     local variable 'e' is assigned to but never used
    
  3. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/patch.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  3. rbtools/commands/patch.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (98 > 79 characters)
    
  4. rbtools/commands/post.py (Diff revision 8)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  5. rbtools/utils/commands.py (Diff revision 8)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  6. rbtools/utils/commands.py (Diff revision 8)
     
     
    Col: 33
     E127 continuation line over-indented for visual indent
    
  7. rbtools/utils/commands.py (Diff revision 8)
     
     
    Col: 59
     W291 trailing whitespace
    
  8. rbtools/utils/commands.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  9. rbtools/utils/commands.py (Diff revision 8)
     
     
    Col: 71
     W291 trailing whitespace
    
  10. 
      
VT
  1. 
      
  2. rbtools/commands/patch.py (Diff revision 8)
     
     
     
     
     
     

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

  3. 
      
VT
  1. 
      
  2. rbtools/utils/commands.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

    1. 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?

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

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

  3. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/post.py (Diff revision 9)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
brennie
  1. 
      
  2. rbtools/commands/patch.py (Diff revision 9)
     
     

    Add a real help description.

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

    One blank line instead of two.

  4. rbtools/templates/default.txt (Diff revision 9)
     
     
     

    Add a blank line between these two.

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

    Undo this change.

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

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

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

    Move this up to the previous line.

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

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

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

    Use e instead of ue.

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

    Use e instead of te.

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

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

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

    More than one of these can fit on a line.

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

    Blank line between a statement and a block

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

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

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

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

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

    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.

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

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

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

    One blank line between these.

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

    Why unset the name here?

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

    Use %s instead for string formatting.

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

    More than one of these can fit on a line.

  22. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/post.py (Diff revision 10)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. rbtools/utils/commands.py (Diff revision 10)
     
     
     'Template' imported but unused
    
  4. rbtools/utils/commands.py (Diff revision 10)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  5. rbtools/utils/commands.py (Diff revision 10)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  6. rbtools/utils/commands.py (Diff revision 10)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/post.py (Diff revision 11)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. rbtools/utils/commands.py (Diff revision 11)
     
     
     undefined name 'ue'
    
  4. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/post.py (Diff revision 12)
     
     

    Please revert this change.

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

    Please revert this change.

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

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

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

    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.

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

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

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

    This method should have a docstring.

    1. Doesn't it already have one?

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

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

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

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

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

    Comments should have proper capitalization and punctuation.

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

    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:
        ...
    ...
    
  11. rbtools/utils/commands.py (Diff revision 12)
     
     

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

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

    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.

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

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

  14. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/land.py (Diff revision 13)
     
     
     'extract_commit_message' imported but unused
    
  3. rbtools/commands/land.py (Diff revision 13)
     
     
     undefined name 'build_commit_message'
    
  4. rbtools/commands/patch.py (Diff revision 13)
     
     
     'extract_commit_message' imported but unused
    
  5. rbtools/commands/patch.py (Diff revision 13)
     
     
     undefined name 'build_commit_message'
    
  6. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/post.py (Diff revision 14)
     
     

    Please revert this change.

  3. rbtools/commands/post.py (Diff revision 14)
     
     

    Revert this change.

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

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

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

    Should have a space before the closing %}

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

    Should have a space before the closing %}

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

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

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

    Returns -> Return.

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

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

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

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

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

    Please add a class docstring.

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

    Indent this 4 more spaces.

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

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

  14. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/land.py (Diff revision 15)
     
     
     

    Alphabetize these.

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

    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.

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

    We use single quotes for strings.

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

    This should be on the previous line.

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

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

    1. 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?

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

    See my other comment.

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

    We use single quotes for strings.

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

    This should be on the previous line.

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

    Use single quotes for all these strings.

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

    Can you put the first %s in double quotes?

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

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

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

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

     * ``foo``: bar
     * ``bar``: baz
    
  14. rbtools/utils/commands.py (Diff revision 15)
     
     

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

    1. yeah template_arg makes sense

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

    Maybe this should this just be template ?

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

    Blank line between these.

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

    Put this on the previous line.

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

    Use a %-formatting expression here.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

  22. rbtools/utils/tests.py (Diff revision 15)
     
     

    Alphabetize these.

  23. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/utils/commands.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/utils/commands.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
david
  1. Just some style nits:

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

    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.

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

    This should be a docstring, not a comment.

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

    No blank line between the docstring and code for methods.

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

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

  6. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. rbtools/utils/commands.py (Diff revision 19)
     
     
     

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

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

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

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

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

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

      mock_review_request.val = 'some_value'
      
  5. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
VT
  1. 
      
  2. rbtools/utils/commands.py (Diff revision 20)
     
     
     

    alphabetical order

  3. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/land.py (Diff revision 21)
     
     
     
     
     
     
     
     
     

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

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

    2. 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).

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

    This isn't needed.

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

    The wrapping should use up more of the line.

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

    Same comments here.

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

    This should be part of the next group of imports.

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

    This should use format strings, instead of string concatenation.

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

    Blank line between these.

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

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

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

    Looks like trim_blocks will easily fit on the previous line.

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

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

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

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

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

    This leaves out the space between sentences.

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

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

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

    Same here.

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

    Missing a trailing period.

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

    Missing a trailing period.

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

    Must be in alphabetical order.

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

    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.

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

    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.

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

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

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

    This should be done in setUp().

  22. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/land.py (Diff revision 22)
     
     
     
     
     
     
     

    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?

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

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

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

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

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

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

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

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

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

    Missing a trailing period.

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

    Should use single quotes where possible.

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

    "Error" shouldn't be capitalized.

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

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

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

    This should just be:

    if not default_templates:
    

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

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

    Blank line here.

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

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

    1. what do you mean by "erroring it out"?

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

    Missing a trailing period.

    "Raises" is maybe the wrong word here?

  14. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/land.py (Diff revision 24)
     
     
     'build_commit_message' imported but unused
    
  3. rbtools/commands/land.py (Diff revision 24)
     
     
    Col: 69
     W291 trailing whitespace
    
  4. rbtools/commands/land.py (Diff revision 24)
     
     
     undefined name 'extract_commit_message'
    
  5. 
      
TI
reviewbot
  1. 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
    
    
  2. rbtools/commands/land.py (Diff revision 25)
     
     
     'build_commit_message' imported but unused
    
  3. rbtools/commands/land.py (Diff revision 25)
     
     
    Col: 69
     W291 trailing whitespace
    
  4. rbtools/commands/land.py (Diff revision 25)
     
     
     undefined name 'extract_commit_message'
    
  5. 
      
TI
reviewbot
  1. 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
    
    
  2. 
      
david
Review request changed

Status: Discarded

Loading...