Add --field option for post command

Review Request #8429 — Created Sept. 23, 2016 and submitted

larmiej
RBTools
master
ff8c0c6...
rbtools, reviewboard, students

This feature updates rbt post to allow for setting custom fields when
creating/updating a review request. It adds a new --field key=value
arguments, which would set extra_data.key=value in the payload. It
currently handles the native fields (description, testing_done,
summary) turning them into aliases of --description, --testing-done,
--summary. It also allows for multiple field arguments to be set

  • added --field description="desc" to an rbt post
  • Ensured that the field description contained "desc"
  • Added multiple fields --field description="high level desc"
    --field summary="short summary" to an rbt post
  • Ensured both argument fields were filled in correctly
  • Added Unit tests
Description From Last Updated

Please include unit tests to ensure that rbt post --field foo=bar ends up with foo=bar sent as form data.

brenniebrennie

Can you make sure to only use single quotes (unless you have a string with a single quote in it)?

brenniebrennie

In your testing done: "high leveldesc" => "high level desc"

brenniebrennie

Can you change your summary to be in the imperitive mood ("Add --field option ...."). Also "option" instead of "argument".

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

fields would probably be a better name.

brenniebrennie

Blank line between these. Also, can we call this key_value_pair? Python uses snake_case_names for variables.

brenniebrennie

Can we include the offending value in the error message?

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

You will want to use six.iteritems(self.options.fields).

brenniebrennie

Hm, I think there's an edgecase here. Suppose I'm trying to set the following field: --field myField="thisstring=has=equals=signs" Your split is ...

mike_conleymike_conley

We might want to have a List of "reserved fields" for cases like this, and use that list instead of ...

mike_conleymike_conley

undefined name 'parser'

reviewbotreviewbot

undefined name 'parser'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 78 W292 no newline at end of file

reviewbotreviewbot

Can we call this create_parser and not have it parse the arguments? Having it set self.options and return the parser ...

brenniebrennie

This should have a docstring, which should have a one-line summary summarizing the purpose of the function, a description going ...

chipx86chipx86

Alphabetical order, please.

brenniebrennie

Imports should be in alphabetical order, but within their correct import group. There are 3 groups: 1) Python Standard Library ...

chipx86chipx86

Proper capitilzation and punctuation.

brenniebrennie

This should be in sentence form. I think it also needs a bit more details. As it is, the help ...

chipx86chipx86

Blank line around blocks (like if, for, etc.).

chipx86chipx86

No blank line here.

chipx86chipx86

Please make this a constant on the class.

brenniebrennie

This should be a tuple, and is better defined on the class (with a doc comment).

chipx86chipx86

Blank line between these.

brenniebrennie

Blank line before for, but not after.

chipx86chipx86

I don't think we need to cast to a list here. If it turns out we do, these statements can ...

chipx86chipx86

No blank line here.

brenniebrennie

Blank line between these.

brenniebrennie

We should use single quotes wherever possible. In Python, they're interchangeable with double quotes, but are "cleaner."

chipx86chipx86

Same comments about blank lines around if.

chipx86chipx86

No blank line here.

brenniebrennie

This won't actually show the key/value pair. You'll want to use %-formatting, e.g.: raise CommandError( 'The --field argument be a ...

brenniebrennie

The first line doesn't seem relevant to the error. When crafting error messages, always try to be as user-friendly as ...

chipx86chipx86

No blank line here.

chipx86chipx86

No blank line here.

brenniebrennie

It can be confusing if both --field description=value and --description=value is set. I think instead of transparently converting these, maybe ...

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these. Also need a file docstring.

brenniebrennie

Alphabetize these.

brenniebrennie

Should be in alphabetical order. This should be: from rbtools.commands import CommandError from rbtools.commands.post import Post from rbtools.utils.testbase import RBTestBase

chipx86chipx86

"RBTools Commands"

brenniebrennie

Should be in sentence form (trailing period). We don't really call these "RBCommands," but we also don't want one giant ...

chipx86chipx86

Unnecessary.

brenniebrennie

Alignment issue.

chipx86chipx86

Make your lines as long as possible under 79 chars.

brenniebrennie

No indentation here. Things should be flush with the beginning of """ for subsequent lines. You can also fit more ...

chipx86chipx86

Unindent this so it is flush with the leading "

brenniebrennie

['rbt', 'post']

brenniebrennie

Best just to set to ['rbt', 'post']. Same with ones below.

chipx86chipx86

Single quotes (here and throughout the rest of the file, except for """).

chipx86chipx86

Make your lines as long as possible under 79 chars.

brenniebrennie

And here.

brenniebrennie

When wrapping lines, it's better to do: self.assertDictEqual(post.options.fields, {...}) However, we're dealing with multi-line dictionaries here, and we should have ...

chipx86chipx86

['rbt', 'post']

brenniebrennie

""" on first line.

brenniebrennie

blank line between these.

brenniebrennie

Col: 25 W503 line break before binary operator

reviewbotreviewbot

Maybe this should be structured as: if (key in self.reserved_fields): if (getattr(self.options, key)): raise... setattr(self.options, key, value) else: extra_fields[key] = ...

mike_conleymike_conley

I'm confused... should we be setting the extra_data field here? Like, it looks like we're assuming that the structure we're ...

mike_conleymike_conley

Also, I don't think we need this first comment - it's already provided on line 10.

mike_conleymike_conley

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

reviewbotreviewbot

Is it possible to add one more test that checks the post_request case to make sure that the extra_data__ prefix ...

mike_conleymike_conley

Probably better: extra_fields['extra_data.%s' % key] = value

mike_conleymike_conley

This should should be --field

brenniebrennie

Args? Returns? Look at other docstrings for examples of the format.

brenniebrennie

Capitlization and period.

brenniebrennie

These are not necessarily provided by extensions. We shoudl also document the fact that description, testing_done and summary can be ...

brenniebrennie

You can do: key, value = key_value_pair

brenniebrennie

How about: The "foo" field was provided by both --foo= and --field foo=. Please only provide one of these. (or ...

brenniebrennie

Blank line between these.

brenniebrennie

Docstrings should be in the imperative mood ("Create and return" instead of "Creates and returns"). We're also working towards including ...

daviddavid

"from custom fields" is kind of confusing outside of the context of this particular change. Maybe "native fields that can ...

daviddavid

This should mention that custom fields get set into the review request's extra_data.

daviddavid

If no fields are specified, what is self.options.fields? Is there a way to avoid this check and just loop over ...

daviddavid

There's an extra line here.

daviddavid

This line can be removed.

daviddavid

Hmm. Shouldn't this be using the extra_data.X keys?

daviddavid

Should be "--field foo=bar"

daviddavid

Should be "--field foo=bar --field desc=new" (instead of "--fields foo=bar -field desc=new". Wrapping is also off.

daviddavid

Should be --field.

daviddavid

--field

daviddavid

Col: 37 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

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

reviewbotreviewbot

Col: 33 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

I don't think you need the extra list() call in here (you can iterate over self.options.fields directly).

daviddavid

You're always assigning self.options.extra_fields inside post_process_fields, so you shouldn't need this check.

daviddavid

Dedent this. The ) should be aligned with the O in Option on the line below.

brenniebrennie

So the flag for testing_done is --testing-done. Make sure that we use the flag names instead of field names in ...

brenniebrennie

Missing a file docstring. (This can be as simple as "Test for RBTools commands.")

brenniebrennie

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

reviewbotreviewbot

Col: 17 E124 closing bracket does not match visual indentation

reviewbotreviewbot

I feel like 'native' is not the correct word here. Perhaps built-in.

brenniebrennie

Remove right paren here.

brenniebrennie

Single quotes.

brenniebrennie

--field

brenniebrennie

Can you use --field here (for consistency)

brenniebrennie

Can you use --field here?

brenniebrennie

Space between the argument name and the type.

daviddavid

We should always put the space at the end of the line instead of the front. If that means wrapping ...

daviddavid

Needs to be an absolute class path.

chipx86chipx86

This isn't a Python-provided module. It needs to go in a third-party import group (new group in-between the Python standard ...

chipx86chipx86

The --field needs to have double backticks on both sides.

chipx86chipx86

Technically, it's "testing_done". Can you test with both variations? --field testing-done=... and --field testing_done=... ?

chipx86chipx86

No need for the else:. It's implied, because the if raises an exception.

chipx86chipx86

We're doing key.replace('-', '_') twice. Can we do it once and assign to a variable?

daviddavid

This file will inevitabley get very long. Can you instead make this rbtools/commands/tests/test_post.py? You'll need a blank rbtools/commands/tests/__init__.py in order ...

chipx86chipx86

This has to be done a lot. Some or all of this would be a good candidate for a utility ...

chipx86chipx86

Should use assertEqual. It implies assertDictEqual when being passed dictionaries. Same below.

chipx86chipx86

Add a trailing comma.

daviddavid

This will never be used without an instance, so @staticmethod isn't really useful. It's better convention to have this just ...

daviddavid

While this does create the parser, it does more than just that (since it sets up the whole command). It ...

daviddavid

This would be better formatted as: post = self.create_arg_parser([ 'description=testing', 'summary=native testing', 'testing-done=No tests', ])

daviddavid

list of unicode

brenniebrennie

"built-in" over "native"

brenniebrennie

Previous line.

brenniebrennie

You can replace this all with: update_fields = self.options.extra_fields.copy()

brenniebrennie

Missing a period.

brenniebrennie

Missinga period.

brenniebrennie

This is internal for tests, so it should be _create_post_command.

brenniebrennie

How about: "Create an argument parser with the given extra fields"

brenniebrennie

list of unicode

brenniebrennie

Missing period. Should be key-value. How about elaborating a bit, e.g. A list of key-value pairs for the field argument. ...

brenniebrennie

"built-in" over "native"

brenniebrennie

"built-in" over "native"

brenniebrennie

"argument"

brenniebrennie

'six' imported but unused

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  3. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  4. 
      
LA
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
LA
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
brennie
  1. 
      
  2. Please include unit tests to ensure that rbt post --field foo=bar ends up with foo=bar sent as form data.

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

    fields would probably be a better name.

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

    Blank line between these.

    Also, can we call this key_value_pair? Python uses snake_case_names for variables.

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

    Can we include the offending value in the error message?

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

    Blank line between these.

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

    Blank line between these.

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

    You will want to use six.iteritems(self.options.fields).

  9. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
    
    
  2. 
      
mike_conley
  1. 
      
  2. rbtools/commands/post.py (Diff revision 4)
     
     
     

    Hm, I think there's an edgecase here.

    Suppose I'm trying to set the following field:

    --field myField="thisstring=has=equals=signs"
    

    Your split is going to split up the value, we'll fail the length check on line 274.

    Even if we didn't check the length, we'd have the other problem where we accidentally set thisstring as the value, since that's key_value_pair[1].

    So I think what we want to do here is split, and make sure that the resulting string is >= 2. Then, I think we want to have the 0th element be the key, and the rest of the elements joined to be the value.

    1. split() accepts an optional parameter for the number of splits. So .split(1) will return ["thisstring", "has=equals=signs"]

    2. I believe you mean .split('=', 1)

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

    We might want to have a List of "reserved fields" for cases like this, and use that list instead of if'ing over each case separately.

  4. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/__init__.py (Diff revision 5)
     
     
     undefined name 'parser'
    
  3. rbtools/commands/__init__.py (Diff revision 5)
     
     
     undefined name 'parser'
    
  4. rbtools/commands/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
RS
  1. Just a couple of comments / questions.

  2. rbtools/commands/post.py (Diff revision 6)
     
     

    I'm super unfamiliar with the six module, so maybe you can disregard this - but isn't the purpose of this module to allow cross compatibility between Python 2 and 3?

    Do we really need this, or could we get away with using dictionary.iteritems().

    1. yeah, we do need it specifically for the cross compability issue

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

    It seems to me like all of these tests assume a happy case scenario. Should we also be concerned about scenarios where you encounter nonsensical key-value pairs? It looks like you're throwing a CommandError in post.py, so there should probably be a unit test for that.

    1. Yeah I could throw some more test on there

  4. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/tests.py (Diff revision 7)
     
     
    Col: 78
     W292 no newline at end of file
    
  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 8)
     
     

    Can we call this create_parser and not have it parse the arguments? Having it set self.options and return the parser is kinda confusing.

    Also this needs a docstring.

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

    Alphabetical order, please.

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

    Proper capitilzation and punctuation.

  5. rbtools/commands/post.py (Diff revision 8)
     
     

    Please make this a constant on the class.

  6. rbtools/commands/post.py (Diff revision 8)
     
     
     

    Blank line between these.

  7. rbtools/commands/post.py (Diff revision 8)
     
     

    No blank line here.

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

    Blank line between these.

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

    No blank line here.

  10. rbtools/commands/post.py (Diff revision 8)
     
     
     
     
     

    This won't actually show the key/value pair. You'll want to use %-formatting, e.g.:

    raise CommandError(
        'The --field argument be a key-value pair as key=value; got "%s" instead.'
        % field
    )
    
  11. rbtools/commands/post.py (Diff revision 8)
     
     

    No blank line here.

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

    Blank line between these.

    Also need a file docstring.

  13. rbtools/commands/tests.py (Diff revision 8)
     
     
     
     

    Alphabetize these.

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

    "RBTools Commands"

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

    Unnecessary.

  16. rbtools/commands/tests.py (Diff revision 8)
     
     
     

    Make your lines as long as possible under 79 chars.

  17. rbtools/commands/tests.py (Diff revision 8)
     
     

    Unindent this so it is flush with the leading "

  18. rbtools/commands/tests.py (Diff revision 8)
     
     

    ['rbt', 'post']

  19. rbtools/commands/tests.py (Diff revision 8)
     
     
     

    Make your lines as long as possible under 79 chars.

  20. rbtools/commands/tests.py (Diff revision 8)
     
     

    And here.

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

    ['rbt', 'post']

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

    """ on first line.

  23. 
      
brennie
  1. 
      
  2. Can you make sure to only use single quotes (unless you have a string with a single quote in it)?

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

    blank line between these.

  4. 
      
chipx86
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 8)
     
     

    This should have a docstring, which should have a one-line summary summarizing the purpose of the function, a description going into details, and documentation on the arguments and return values.

    The RBTools codebase is old, so there are plenty of methods that aren't documented to the degree that they should be, but new code should follow the modern conventions.

    Those are documented here: https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/

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

    Imports should be in alphabetical order, but within their correct import group. There are 3 groups:

    1) Python Standard Library modules (which is this current existing group).
    2) Third-party modules (six is one of these)
    3) Project modules (rbtools.*)

    There's a blank line between each, and import ... statements come before from ... imports in each group.

    There's a special group (group 0) that is the from __future__ import ... statements, which would be before all other groups.

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

    This should be in sentence form. I think it also needs a bit more details. As it is, the help text I think will only make sense to those who are already familiar with what this flag does.

    I'd propose: "Sets custom fields (provided by extensions) on the review request."

    The metavar can then document the format: metavar="FIELD_NAME=VALUE"

  5. rbtools/commands/post.py (Diff revision 8)
     
     
     

    Blank line around blocks (like if, for, etc.).

  6. rbtools/commands/post.py (Diff revision 8)
     
     
     
     

    No blank line here.

  7. rbtools/commands/post.py (Diff revision 8)
     
     

    This should be a tuple, and is better defined on the class (with a doc comment).

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

    Blank line before for, but not after.

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

    I don't think we need to cast to a list here. If it turns out we do, these statements can be combined.

  10. rbtools/commands/post.py (Diff revision 8)
     
     

    We should use single quotes wherever possible. In Python, they're interchangeable with double quotes, but are "cleaner."

  11. rbtools/commands/post.py (Diff revision 8)
     
     
     
     

    Same comments about blank lines around if.

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

    The first line doesn't seem relevant to the error.

    When crafting error messages, always try to be as user-friendly as possible. "Key value pair" and "key" are probably not phrases/terms to use, as they're more internal terminology. Instead, we can safely say "The --field argument should be in the form of: --field name=value"

  13. rbtools/commands/post.py (Diff revision 8)
     
     
     
     

    No blank line here.

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

    It can be confusing if both --field description=value and --description=value is set. I think instead of transparently converting these, maybe we want to just tell the user to use e.g. --description instead.

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

    Blank line between these.

  16. rbtools/commands/tests.py (Diff revision 8)
     
     
     
     

    Should be in alphabetical order. This should be:

    from rbtools.commands import CommandError
    from rbtools.commands.post import Post
    from rbtools.utils.testbase import RBTestBase
    
  17. rbtools/commands/tests.py (Diff revision 8)
     
     

    Should be in sentence form (trailing period).

    We don't really call these "RBCommands," but we also don't want one giant class for all commands. Instead, let's have a class per command. So this can be class PostCommandTests, and the description can reference rbt post.

  18. rbtools/commands/tests.py (Diff revision 8)
     
     
     

    Alignment issue.

  19. rbtools/commands/tests.py (Diff revision 8)
     
     
     
     

    No indentation here. Things should be flush with the beginning of """ for subsequent lines. You can also fit more on the first line here.

    Each one of these should also mention "rbt post," like:

    """Testing rbt post with <condition>"""

    When a docstring can fit on one line, put the final """ on the same line.

    These all apply below.

  20. rbtools/commands/tests.py (Diff revision 8)
     
     

    Best just to set to ['rbt', 'post'].

    Same with ones below.

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

    Single quotes (here and throughout the rest of the file, except for """).

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

    When wrapping lines, it's better to do:

    self.assertDictEqual(post.options.fields,
                         {...})
    

    However, we're dealing with multi-line dictionaries here, and we should have one entry per line for readability/maintainability. We also want to keep things at nice 4-space indentation levels. So use this form instead:

    self.assertDictEqual(
        post.options.fields,
        {
            'foo': 'bar',
            'desc': 'new',
         })
    

    Note also the trailing comma on the last entry in the dictionary. We want these for dictionaries, lists, tuples, etc. so that future updates can simply add a line and not modify the previous to add a comma.

  23. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/post.py (Diff revision 9)
     
     
    Col: 25
     W503 line break before binary operator
    
  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
mike_conley
  1. 
      
  2. rbtools/commands/post.py (Diff revision 10)
     
     

    You might want to double-check with chipx86, david or brennie, but I'm not sure "native" is the right word here. Maybe built-in?

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

    So the rule is, we can set these built-in/native fields iff they haven't already been set?

    I just want to make sure that's explicit and desired.

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

    Maybe this should be structured as:

    if (key in self.reserved_fields):
        if (getattr(self.options, key)):
          raise...
        setattr(self.options, key, value)
    else:
      extra_fields[key] = value
    

    This helps reduce some duplication.

  5. rbtools/commands/post.py (Diff revision 10)
     
     

    I'm confused... should we be setting the extra_data field here? Like, it looks like we're assuming that the structure we're updating is like:

    # For --field foo="bar"
    {
      'target_groups': [],
      'target_people': [],
      'foo': "bar",
    }
    

    When I think it's supposed to be:

    # For --field foo="bar"
    {
      'target_groups': [],
      'target_people': [],
      'extra_data__foo': "bar",
    }
    

    See https://github.com/reviewboard/rbtools/commit/e35445f43ef97cc7fb358d09c5d0d72a19f83904 for details.

    1. Whoops, brennie pointed out I made an error here. This should not be extra_data__foo, but extra_data.foo.

  6. 
      
mike_conley
  1. 
      
  2. rbtools/commands/tests.py (Diff revision 10)
     
     

    Also, I don't think we need this first comment - it's already provided on line 10.

  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/post.py (Diff revision 11)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. 
      
LA
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
mike_conley
  1. 
      
  2. rbtools/commands/tests.py (Diff revision 12)
     
     

    Is it possible to add one more test that checks the post_request case to make sure that the extra_data__ prefix is being set?

  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
mike_conley
  1. This looks great to me! Thanks John!

  2. rbtools/commands/post.py (Diff revision 13)
     
     

    Probably better:

    extra_fields['extra_data.%s' % key] = value

  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/post.py (Diff revision 14)
     
     

    This should should be --field

  3. 
      
LA
brennie
  1. 
      
  2. In your testing done: "high leveldesc" => "high level desc"

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

    Args? Returns? Look at other docstrings for examples of the format.

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

    Capitlization and period.

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

    These are not necessarily provided by extensions. We shoudl also document the fact that description, testing_done and summary can be set this way.

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

    You can do:

    key, value = key_value_pair
    
  7. rbtools/commands/post.py (Diff revision 15)
     
     
     

    How about:

    The "foo" field was provided by both --foo= and --field foo=. Please only provide one of these. (or similar)

    Also, missing a period.

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

    Blank line between these.

  9. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
david
  1. I don't see anything in your testing about actually testing custom fields (it looks like you just tested with the description). Can you run it with a custom field name and verify in the devserver's database that it set the extra_data correctly?

  2. rbtools/commands/__init__.py (Diff revision 16)
     
     

    Docstrings should be in the imperative mood ("Create and return" instead of "Creates and returns").

    We're also working towards including more detailed information about function arguments, return values, etc. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#special-sections (you'll need "Args" and "Returns" sections here).

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

    "from custom fields" is kind of confusing outside of the context of this particular change. Maybe "native fields that can be set using the --field argument"?

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

    This should mention that custom fields get set into the review request's extra_data.

  5. rbtools/commands/post.py (Diff revision 16)
     
     

    If no fields are specified, what is self.options.fields? Is there a way to avoid this check and just loop over a potentially empty list in order to avoid the extra level of indentation?

    1. From my understanding, self.options.fields will be empty and will not pass the if condition.
      Unsure if I answered your question.

    2. If it's an empty list, then we can just do:

      for field in self.options.fields:
          ...
      

      instead of:

      if self.options.fields:
          for field in self.options.fields:
              ...
      
    3. self.options.fields is a NoneType when empty so it throws a TypeError: 'NoneType' object is not iterable

    4. OK. In that case we can do something like this:

      if self.options.fields is None:
          self.options.fields = []
      
      for field in self.options.fields:
          ...
      
    5. I agree with you and will make the change, may i add that if i do set self.options.fields = [],it may not be clear when writing new code with regards to options.fields because it is assumed that an options argument is None when it is not set and it is checked that way thorught the code. Although, if empty list == false and will make the condition checking still valid.

  6. rbtools/commands/post.py (Diff revision 16)
     
     

    There's an extra line here.

  7. rbtools/commands/post.py (Diff revision 16)
     
     

    This line can be removed.

  8. rbtools/commands/post.py (Diff revision 16)
     
     

    Hmm. Shouldn't this be using the extra_data.X keys?

    1. Yes, it does use it. Ref:line 297

    2. Ah, I missed the re-assignment to self.options.fields. This can be made clearer. Instead of having self.options.fields come in as a list and exit as a dictionary, can we assign directly to a new value (maybe self.options.extra_fields)? That way it's always clear which is which without having to trace trough the code.

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

    Should be "--field foo=bar"

  10. rbtools/commands/tests.py (Diff revision 16)
     
     
     
     

    Should be "--field foo=bar --field desc=new" (instead of "--fields foo=bar -field desc=new". Wrapping is also off.

  11. rbtools/commands/tests.py (Diff revision 16)
     
     

    Should be --field.

  12. rbtools/commands/tests.py (Diff revision 16)
     
     

    --field

  13. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/post.py (Diff revision 18)
     
     
    Col: 37
     E131 continuation line unaligned for hanging indent
    
  3. rbtools/commands/tests.py (Diff revision 18)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  4. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/post.py (Diff revision 20)
     
     
    Col: 33
     E131 continuation line unaligned for hanging indent
    
  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/post.py (Diff revision 21)
     
     

    I don't think you need the extra list() call in here (you can iterate over self.options.fields directly).

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

    You're always assigning self.options.extra_fields inside post_process_fields, so you shouldn't need this check.

  4. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. 
      
  2. Can you change your summary to be in the imperitive mood ("Add --field option ....").

    Also "option" instead of "argument".

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

    Dedent this. The ) should be aligned with the O in Option on the line below.

    1. review bot is flagging this change

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

    So the flag for testing_done is --testing-done. Make sure that we use the flag names instead of field names in the --flag bit

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

    Missing a file docstring.

    (This can be as simple as "Test for RBTools commands.")

    1. There was an issue created to remove the doc string after you told me to put it before. That a doc string already existed on line 9 in the class. Should I still have both in there?

  6. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/post.py (Diff revision 23)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. rbtools/commands/post.py (Diff revision 24)
     
     
    Col: 17
     E124 closing bracket does not match visual indentation
    
  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/post.py (Diff revision 25)
     
     

    I feel like 'native' is not the correct word here. Perhaps built-in.

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

    Remove right paren here.

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

    Single quotes.

  5. rbtools/commands/tests.py (Diff revision 25)
     
     

    --field

  6. rbtools/commands/tests.py (Diff revision 25)
     
     

    Can you use --field here (for consistency)

  7. rbtools/commands/tests.py (Diff revision 25)
     
     

    Can you use --field here?

  8. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
david
  1. Just a couple small nits.

  2. rbtools/commands/__init__.py (Diff revision 26)
     
     

    Space between the argument name and the type.

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

    We should always put the space at the end of the line instead of the front. If that means wrapping it at an earlier point, that's fine.

  4. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/tests.py
        rbtools/commands/__init__.py
    
    
  2. 
      
david
  1. Just a couple tiny nits left.

  2. rbtools/commands/post.py (Diff revision 27)
     
     
     
     
     
     
     
     
     

    We're doing key.replace('-', '_') twice. Can we do it once and assign to a variable?

  3. rbtools/commands/tests.py (Diff revision 27)
     
     

    Add a trailing comma.

  4. 
      
chipx86
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 27)
     
     

    Needs to be an absolute class path.

    1. what do you mean by absolute class path, could I have an example?

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

    This isn't a Python-provided module. It needs to go in a third-party import group (new group in-between the Python standard library imports and the rbtools imports).

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

    The --field needs to have double backticks on both sides.

  5. rbtools/commands/post.py (Diff revision 27)
     
     

    Technically, it's "testing_done". Can you test with both variations? --field testing-done=... and --field testing_done=... ?

  6. rbtools/commands/post.py (Diff revision 27)
     
     
     
     
     
     
     

    No need for the else:. It's implied, because the if raises an exception.

  7. rbtools/commands/tests.py (Diff revision 27)
     
     

    This file will inevitabley get very long. Can you instead make this rbtools/commands/tests/test_post.py?

    You'll need a blank rbtools/commands/tests/__init__.py in order to make this a module.

  8. rbtools/commands/tests.py (Diff revision 27)
     
     
     
     
     
     
     

    This has to be done a lot. Some or all of this would be a good candidate for a utility function on this test class.

  9. rbtools/commands/tests.py (Diff revision 27)
     
     

    Should use assertEqual. It implies assertDictEqual when being passed dictionaries.

    Same below.

  10. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/tests/test_post.py (Diff revision 28)
     
     

    This will never be used without an instance, so @staticmethod isn't really useful. It's better convention to have this just be an instance method that takes an unused self parameter.

  3. rbtools/commands/tests/test_post.py (Diff revision 28)
     
     

    While this does create the parser, it does more than just that (since it sets up the whole command). It might be better to name this something like create_post_command.

  4. rbtools/commands/tests/test_post.py (Diff revision 28)
     
     
     
     
     

    This would be better formatted as:

    post = self.create_arg_parser([
        'description=testing',
        'summary=native testing',
        'testing-done=No tests',
    ])
    
  5. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
  2. 
      
david
  1. I'm happy with this. I'm going to try to get another mentor to give it a last glance to make sure there's nothing I missed.

  2. 
      
brennie
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 29)
     
     

    list of unicode

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

    "built-in" over "native"

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

    Previous line.

  5. rbtools/commands/post.py (Diff revision 29)
     
     
     
     
     
     

    You can replace this all with:

    update_fields = self.options.extra_fields.copy()
    
    1. I don't think that's quite the best option because it forces it to be the first one we copy over. That said, we could do:

      update_fields.update(self.options.extra_fields)
      

      instead of the loop.

  6. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    Missing a period.

  7. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    Missinga period.

  8. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    This is internal for tests, so it should be _create_post_command.

  9. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    How about: "Create an argument parser with the given extra fields"

  10. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    list of unicode

  11. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    Missing period. Should be key-value.

    How about elaborating a bit, e.g.

    A list of key-value pairs for the field argument.
    
    Each pair should be of the form key=value.
    
  12. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    "built-in" over "native"

  13. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    "built-in" over "native"

  14. rbtools/commands/tests/test_post.py (Diff revision 29)
     
     

    "argument"

  15. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
  2. rbtools/commands/post.py (Diff revision 30)
     
     
     'six' imported but unused
    
  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/tests/test_post.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
    
    Ignored Files:
        rbtools/commands/tests/__init__.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
LA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (f4aabf5)
Loading...