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



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

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

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

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



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

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



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

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

fields would probably be a better name.

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

Can we include the offending value in the error message?

Blank line between these.

Blank line between these.

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

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



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

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.

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.

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

 undefined name 'parser'

 undefined name 'parser'

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

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

Just a couple of comments / questions.

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

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.

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

Col: 78
 W292 no newline at end of file

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

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.

Alphabetical order, please.

Proper capitilzation and punctuation.

Please make this a constant on the class.

Blank line between these.

No blank line here.

Blank line between these.

No blank line here.

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
)

No blank line here.

Blank line between these.
Also need a file docstring.

Alphabetize these.

"RBTools Commands"

Unnecessary.

Make your lines as long as possible under 79 chars.

Unindent this so it is flush with the leading "

['rbt', 'post']

Make your lines as long as possible under 79 chars.

And here.

['rbt', 'post']

""" on first line.

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

blank line between these.

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/

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.

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"

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

No blank line here.

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

Blank line before for, but not after.

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

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

Same comments about blank lines around if.

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"

No blank line here.

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.

Blank line between these.

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

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.

Alignment issue.

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.

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

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

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.

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

Col: 25
 W503 line break before binary operator

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

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?

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.

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.

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.

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

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

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

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

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?

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

This looks great to me! Thanks John!

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

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

This should should be --field

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

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

Capitlization and period.

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

You can do:

key, value = key_value_pair

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.

Blank line between these.

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

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

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?

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

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

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

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?

There's an extra line here.

This line can be removed.

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

Should be "--field foo=bar"

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

Should be --field.

--field

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

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

Col: 37
 E131 continuation line unaligned for hanging indent

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

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

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

Col: 33
 E131 continuation line unaligned for hanging indent

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

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

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

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