Summary:

Fixed issues with rbtools argument parsing on Python 3 when running '--help'

Review Request #10856 — Created Jan. 24, 2020 and submitted Feb. 12, 2020, 1:37 p.m.

Information
Owner:	kpatenio
Repository:	RBTools
Branch:	release-2.0.x
Bugs:
Depends On:
Commit:	392c050...
Reviewers
Groups:	rbtools
People:

Description

On python 2, running rbt --help <command> and rbt <command> --help
are equivalent. However, running rbt <command> --help does not work
on python 3. This happens because byte strings are utilized in the
code to see if --help or -h are passed in as arguments. They are
never found because the code is trying to find b'--help' or b'-h'.
In python 2, b'--help' is equivalent to '--help', but in python 3,
this is not the case.

To fix this issue, the byte strings b'--help' and b’-h’ have been
changed to python 3 compatible strings (i.e. --help and -h). Unit
tests have also been added to ensure that all possible positions and
cases of the 'help' argument work for rbtools.(ex. rbt —help <command>,
rbt <command> -h, rbt help <command>, etc.).

All tests were run using both python 2.7.17 and python 3.7.6.

Testing Done

Test cases run on python 2.7.17 and 3.7.6:

Test case where rbt help <command> is run

Test case where rbt <command> --help or rbt <command> -h is run

Test case where rbt --help <command> or rbt -h <command> is run

Test case where an invalid rbt command is run with help

Test case where multiple arguments are entered

Issues

Description	From	Last Updated
Can you also detail which versions of Python all your tests have been run under? We need to ensure full …	chipx86	Jan. 25, 2020, 9:30 a.m.
Can you wrap the change description at 79 columns? It would also be nice to use backticks whenever you're showing …	chipx86	Jan. 25, 2020, 9:35 a.m.
E501 line too long (91 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (94 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (88 > 79 characters)	reviewbot	Jan. 24, 2020, 8:58 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
E501 line too long (88 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Jan. 24, 2020, 8:59 p.m.
I think this docstring and file could be named in a more descriptive way eg. Tests for RBTools help command …	hxqlin	Jan. 25, 2020, 12:18 a.m.
Similar issue about naming here - I think something like HelpCommandTests would be better	hxqlin	Jan. 25, 2020, 12:18 a.m.
I think this could be better named as test_help_command. Ordering it that way would make it more consistent with the …	hxqlin	Jan. 25, 2020, 12:18 a.m.
Maybe this could be pulled out into a function since you use it a few times	hxqlin	Jan. 25, 2020, 12:18 a.m.
This snippet feels like it could also be a function since you use it a few times. Also, maybe command_string …	hxqlin	Jan. 25, 2020, 12:19 a.m.
Same as above	hxqlin	Jan. 25, 2020, 12:19 a.m.
Minor but maybe naming test_help to be test_command would be better here to be more consistent with your first test …	hxqlin	Jan. 25, 2020, 12:19 a.m.
Same as above	hxqlin	Jan. 25, 2020, 12:19 a.m.
Same as above	hxqlin	Jan. 25, 2020, 12:19 a.m.
Same as above	hxqlin	Jan. 25, 2020, 12:19 a.m.
Since you're testing help already, it might be a good idea to add tests for when there are more than …	hxqlin	Jan. 25, 2020, 12:19 a.m.
W293 blank line contains whitespace	reviewbot	Jan. 25, 2020, 12:20 a.m.
W293 blank line contains whitespace	reviewbot	Jan. 25, 2020, 12:20 a.m.
W293 blank line contains whitespace	reviewbot	Jan. 25, 2020, 12:20 a.m.
W293 blank line contains whitespace	reviewbot	Jan. 25, 2020, 12:20 a.m.
W391 blank line at end of file	reviewbot	Jan. 25, 2020, 12:20 a.m.
This is wrapping way too early. Can you make sure it wraps at 79 characters? Same with docstrings below.	chipx86	Jan. 25, 2020, 9:30 a.m.
I think you can go ahead and just use these strings inline. It results in less code, and is easier …	chipx86	Jan. 25, 2020, 9:30 a.m.
Blank line between blocks/statements. Same below.	chipx86	Jan. 25, 2020, 9:30 a.m.
Docstring summaries are limited to one line. Same below.	chipx86	Jan. 25, 2020, 9:30 a.m.
The string type is going to be required. Should be unicode. Same below.	chipx86	Jan. 25, 2020, 9:30 a.m.
"Returns" blocks are in the form of: Returns: type: description So probably unicode for the type.	chipx86	Jan. 25, 2020, 9:30 a.m.
We use %-formatting, instead of .format(). It's technically faster.	chipx86	Jan. 25, 2020, 9:30 a.m.
Blank line between each argument.	chipx86	Jan. 25, 2020, 9:30 a.m.
bool	chipx86	Jan. 25, 2020, 9:30 a.m.
bool	chipx86	Jan. 25, 2020, 9:30 a.m.
W291 trailing whitespace	reviewbot	Jan. 25, 2020, 9:34 a.m.
W291 trailing whitespace	reviewbot	Jan. 25, 2020, 9:34 a.m.
W291 trailing whitespace	reviewbot	Jan. 25, 2020, 9:34 a.m.
W291 trailing whitespace	reviewbot	Jan. 25, 2020, 9:34 a.m.
I've been thinking about these code blocks, which are almost universal in your code. I think what would be better …	chipx86	Jan. 30, 2020, 11:19 a.m.
How about making this one statement? Same below.	chipx86	Jan. 30, 2020, 11:19 a.m.
This could be one line now. Same with ones below.	chipx86	Jan. 30, 2020, 11:19 a.m.
Sorry, I wasn't clear in my description above. We'd want a blank line when separating blocks on the same indentation …	chipx86	Jan. 30, 2020, 11:18 a.m.
This can be one statement.	chipx86	Feb. 8, 2020, 12:05 a.m.
Here, and below, it'll be a lot easier to debug if we're passing in the command directly to _check_help_output. The …	chipx86	Feb. 8, 2020, 12:05 a.m.
This should be "Check". We use a form of "Do this, do that" rather than "Does this, does that." in …	chipx86	Feb. 8, 2020, 12:04 a.m.
The type should be (bool, optional), since it's not required.	chipx86	Feb. 8, 2020, 12:05 a.m.
True should be shown as a literal, using double-backticks.	chipx86	Feb. 8, 2020, 12:05 a.m.
Functions that don't return a value shouldn't have a Returns section.	chipx86	Feb. 8, 2020, 12:05 a.m.
This seems fragile. I'm not wild about it. The idea is to have a version of command that didn't have …	chipx86	Feb. 8, 2020, 12:05 a.m.
Rather than defining the variable, you can move the assertIn into those if statements.	chipx86	Feb. 8, 2020, 12:05 a.m.
This is only here for the execute command, right? In this case, let's only wrap as much as we need …	chipx86	Feb. 8, 2020, 12:05 a.m.

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (91 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (91 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (91 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (94 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (92 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (92 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (92 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (88 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (92 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (92 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (92 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (88 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
rbtools/commands/tests/test_main.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```

Change Summary:

Fixed linting issues

Testing Done:

~		Testing Done:
~		* Test case where 'rbt help <command>' is run
~		* Test case where 'rbt <command> --help' or 'rbt <command> -h' is run
~		* Test case where 'rbt --help <command>' or 'rbt -h <command>' is run
~		* Test case where an invalid rbt command is run with 'help'
	~	Testing Done:
	~	Test case where 'rbt help <command>' is run
	~	Test case where 'rbt <command> --help' or 'rbt <command> -h' is run
	~	Test case where 'rbt --help <command>' or 'rbt -h <command>' is run
	~	Test case where an invalid rbt command is run with 'help'

Commit:

-	f436426d2b9f182dd24ca32d2d27096881265c15
+	a7f62a85769b8ca6fac60ae945edec61c742c533

Diff:

Revision 2 (+107 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/tests/test_main.py (Diff revision 2)

Show all issues

I think this docstring and file could be named in a more descriptive way eg. Tests for RBTools help command and test_help.py. That way it's also consistent with the other test file I see in that directory (test_post.py).

rbtools/commands/tests/test_main.py (Diff revision 2)

Show all issues

Similar issue about naming here - I think something like HelpCommandTests would be better

rbtools/commands/tests/test_main.py (Diff revision 2)

Show all issues

I think this could be better named as test_help_command. Ordering it that way would make it more consistent with the couple of other tests with a similar pattern in the name in test_post.py

rbtools/commands/tests/test_main.py (Diff revision 2)

Show all issues

Maybe this could be pulled out into a function since you use it a few times

rbtools/commands/tests/test_main.py (Diff revision 2)

Show all issues

This snippet feels like it could also be a function since you use it a few times.
Also, maybe command_string and test could be renamed to something more descriptive, such as expected_command_string and test_result - it wasn't very clear to me that test contained the output of executing the commands.

rbtools/commands/tests/test_main.py (Diff revision 2)
Show all issues
```
Same as above
```

rbtools/commands/tests/test_main.py (Diff revision 2)

Show all issues

Minor but maybe naming test_help to be test_command would be better here to be more consistent with your first test where you referred to a single command as test_command or changing the first one to test_help (but imo test_command is easier to understand)

rbtools/commands/tests/test_main.py (Diff revision 2)
Show all issues
```
Same as above
```
rbtools/commands/tests/test_main.py (Diff revision 2)
Show all issues
```
Same as above
```
rbtools/commands/tests/test_main.py (Diff revision 2)
Show all issues
```
Same as above
```

rbtools/commands/tests/test_main.py (Diff revision 2)

Show all issues

Since you're testing help already, it might be a good idea to add tests for when there are more than than three arguments eg. rbt help alias abcd and also less than eg. rbt help

Change Summary:

Addressed review changes and added a new test case

Testing Done:

		Testing Done:
		Test case where 'rbt help <command>' is run
		Test case where 'rbt <command> --help' or 'rbt <command> -h' is run
		Test case where 'rbt --help <command>' or 'rbt -h <command>' is run
~		Test case where an invalid rbt command is run with 'help'
	~	Test case where an invalid rbt command is run with 'help'
	+	Test case where multiple arguments are entered

Commit:

-	a7f62a85769b8ca6fac60ae945edec61c742c533
+	883fd61c35b3699bdeced4956cf8d3f739ea5ac6

Diff:

Revision 3 (+194 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_main.py (Diff revision 3)
Show all issues
```
W293 blank line contains whitespace
```
rbtools/commands/tests/test_main.py (Diff revision 3)
Show all issues
```
W293 blank line contains whitespace
```
rbtools/commands/tests/test_main.py (Diff revision 3)
Show all issues
```
W293 blank line contains whitespace
```
rbtools/commands/tests/test_main.py (Diff revision 3)
Show all issues
```
W293 blank line contains whitespace
```
rbtools/commands/tests/test_main.py (Diff revision 3)
Show all issues
```
W391 blank line at end of file
```

Commit:

-	883fd61c35b3699bdeced4956cf8d3f739ea5ac6
+	382d4bb8d8b664d107f8267aff468ba33986084c

Diff:

Revision 4 (+189 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

First pass, addressing some of the syntax and standards stuff.

Show all issues

Can you also detail which versions of Python all your tests have been run under? We need to ensure full coverage.

Show all issues

Can you wrap the change description at 79 columns?

It would also be nice to use backticks whenever you're showing commands, arguments, or Python code, to help readability.

kpatenio Jan. 25, 2020, 9:32 a.m.

Just curious, is there a way to show a 79 characters guideline for review board? I can't tell by sight alone
whether or not my decriptions are 79 characters or shorter. (Except for IDEs. I'm able to do that easily).

And whoops, yeah I forgot about changing the backticks for the description. The reason I used single quotes
was because I originally made this my commit message. Whenever I committed, anything wrapped in backticks ran
as if they ran on the command line (the output would be printed onto the commit message).

Shoule I leave the single quotes on my commit message? Or should I update it to make it not rely on single quotes?

(Also thanks for the review!)

chipx86 Jan. 26, 2020, 4:25 p.m.

No there's not. It's been a long-standing feature request that I'd love to have in in some form, both for the description fields and for the diff viewer.

Part of the problem is customizability. Once you start highlighting a column limitation, people will want to customize what column that is, and we've never figured out what the right approach is there. If it's per-user, great (and probably the right place to start). If people want to enforce it, it gets tricky.

Regarding the backticks, are you saying that when you write a commit message and include backticks, the code in those backticks get executed on save? If so, that's very weird, and we should diagnose that in chat.

The quotes otherwise look fine right now in the Description.

kpatenio Jan. 26, 2020, 4:35 p.m.

I see, good to know! And yes, the code in the backticks would get executed once I saved a commit message. For example, I had help (in backticks) somewhere in my commit message,. After entering my commit message, instead of displaying 'help', it printed the following:

GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin18)
These shell commands are defined internally.  Type `help' to see this list.
Type `help name' to find out more about the function `name'.
Use `info bash' to find out more about the shell in general.
Use `man -k' or `info' to find out more about commands not in this list.

...


Quite odd indeed.

rbtools/commands/tests/test_main.py (Diff revision 4)

Show all issues

This is wrapping way too early. Can you make sure it wraps at 79 characters?

Same with docstrings below.

rbtools/commands/tests/test_main.py (Diff revision 4)

Show all issues

I think you can go ahead and just use these strings inline. It results in less code, and is easier to read through and understand.

Same with tests below.

rbtools/commands/tests/test_main.py (Diff revision 4)
Show all issues
```
Blank line between blocks/statements.
Same below.
```

rbtools/commands/tests/test_main.py (Diff revision 4)

Show all issues

Docstring summaries are limited to one line.

Same below.

rbtools/commands/tests/test_main.py (Diff revision 4)

Show all issues

The string type is going to be required. Should be unicode.

Same below.

rbtools/commands/tests/test_main.py (Diff revision 4)

Show all issues

"Returns" blocks are in the form of:

Returns:
    type:
    description

So probably unicode for the type.

rbtools/commands/tests/test_main.py (Diff revision 4)

Show all issues

We use %-formatting, instead of .format(). It's technically faster.

rbtools/commands/tests/test_main.py (Diff revision 4)
Show all issues
```
Blank line between each argument.
```
rbtools/commands/tests/test_main.py (Diff revision 4)
Show all issues
```
bool
```
rbtools/commands/tests/test_main.py (Diff revision 4)
Show all issues
```
bool
```

Change Summary:

Addressed syntax and styling reviews

Description:

~		On python 2, running 'rbt --help <command>' and 'rbt <command> --help' are equivalent. However, running
~		'rbt <command> --help' does not work on python 3. This happens because byte strings are utilized in the code to see if
~		'--help' or '-h' are passed in as arguments. However, '--help' and 'h' are never found because the code is trying to find
~		b'--help' or b'-h'. In python 2, b'--help' is equivalent to '--help', but in python 3, this is not the case.
	~	On python 2, running `rbt --help <command>` and `rbt <command> --help`
	~	are equivalent. However, running `rbt <command> --help` does not work
	~	on python 3. This happens because byte strings are utilized in the
	~	code to see if `--help` or `-h` are passed in as arguments. They are
	+	never found because the code is trying to find `b'--help`' or `b'-h'`.
	+	In python 2, `b'--help'` is equivalent to `'--help'`, but in python 3,
	+	this is not the case.

~		To fix this issue, the byte strings b'--help' and b’-h’ have been changed to python 3 compatible strings (i.e. '--help' and ‘-h’).
~		Unit tests have also been added to ensure that all possible positions and cases of the 'help' argument work for rbtools.
~		(ex. 'rbt —help <command>', 'rbt <command> -h', 'rbt help <command>', etc. )
	~	To fix this issue, the byte strings `b'--help'` and `b’-h’` have been
	~	changed to python 3 compatible strings (i.e. `--help` and `-h`). Unit
	~	tests have also been added to ensure that all possible positions and
	+	cases of the 'help' argument work for rbtools.(ex. `rbt —help <command>`,
	+	`rbt <command> -h`, `rbt help <command>`, etc.).
	+
	+	All tests were run using both python 2.7.17 and python 3.7.6.

Testing Done:

~		Testing Done:
~		Test case where 'rbt help <command>' is run
~		Test case where 'rbt <command> --help' or 'rbt <command> -h' is run
~		Test case where 'rbt --help <command>' or 'rbt -h <command>' is run
~		Test case where an invalid rbt command is run with 'help'
	~	Test cases run on python 2.7.17 and 3.7.6:
	~	Test case where `rbt help <command>` is run
	~	Test case where `rbt <command> --help` or `rbt <command> -h` is run
	~	Test case where `rbt --help <command>` or `rbt -h <command>` is run
	~	Test case where an invalid rbt command is run with `help`
		Test case where multiple arguments are entered

Commit:

-	382d4bb8d8b664d107f8267aff468ba33986084c
+	fbdc829ff4b1d6c0ea256a75a9bc59ea473a2687

Diff:

Revision 5 (+185 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_main.py (Diff revision 5)
Show all issues
```
W291 trailing whitespace
```
rbtools/commands/tests/test_main.py (Diff revision 5)
Show all issues
```
W291 trailing whitespace
```
rbtools/commands/tests/test_main.py (Diff revision 5)
Show all issues
```
W291 trailing whitespace
```
rbtools/commands/tests/test_main.py (Diff revision 5)
Show all issues
```
W291 trailing whitespace
```

Change Summary:

Removed trailing whitespaces

Commit:

-	fbdc829ff4b1d6c0ea256a75a9bc59ea473a2687
+	1fce31258b1216379b42dc5759259f8681dbadf9

Diff:

Revision 6 (+185 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Replaced variables 'invalid_rbt_command' with strings 'invalid'

Commit:

-	1fce31258b1216379b42dc5759259f8681dbadf9
+	ba444c99cbf615c716ba3c58dc50b6a385cc949e

Diff:

Revision 7 (+184 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

kpatenio Jan. 30, 2020, 11:22 a.m.

Hi Christian! I updated my code and it looks so much cleaner now. (Deleted about 80 lines of code!). I removed some code relating to error_message (like seen here) since I found them redundant; the assert error messages were enough in my opinion. Thanks for your help!

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

Show all issues

I've been thinking about these code blocks, which are almost universal in your code. I think what would be better would be to have a single command that takes care of executing these tests. Something like a _check_help_output() method, which takes the command to run and the invalid parameter.

That would take care of the try/execute/except block, would take care of creating the fail message (you could just roll that logic up into this method), and the assertion.

The assertion itself can then become an assertIn, rather than an assertTrue(... in ...), which is what it basically is today. That allows us to leverage the standard behavior in unit tests for checking for a needle in a haystack, taking advantage of the enhanced failure information that provides.

This would simplify the tests greatly. Each test for a command would basically be a call. You could remove the for loops and just call this with each variant (which really helps when debugging test failures -- it's harder when loops are involved).

rbtools/commands/tests/test_main.py (Diff revision 7)
Show all issues
```
How about making this one statement?
Same below.
```

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

Show all issues

This could be one line now.

Same with ones below.

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

Show all issues

Sorry, I wasn't clear in my description above. We'd want a blank line when separating blocks on the same indentation level, but you wouldn't have one between the start of a block and that block's first statement.

This is fine:

test_rbt_commands = [
    ...
]

for test_command in test_rbt_commands:
    try:
        ...

Change Summary:

Refactored code to be simplified

Commit:

-	ba444c99cbf615c716ba3c58dc50b6a385cc949e
+	d06066f5d11f28a571fadd2fb44f6ee97af8a6cf

Diff:

Revision 8 (+106 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

rbtools/commands/tests/test_main.py (Diff revision 8)
Show all issues
```
This can be one statement.
```

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

Show all issues

Here, and below, it'll be a lot easier to debug if we're passing in the command directly to _check_help_output. The list doesn't benefit us in any way, since we unrolled the loops, and can actually lead to future copy/paste bugs (if we skip an index or miss an item in the list).

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

Show all issues

This should be "Check". We use a form of "Do this, do that" rather than "Does this, does that." in the summary.

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

Show all issues

The type should be (bool, optional), since it's not required.

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

Show all issues

True should be shown as a literal, using double-backticks.

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

Show all issues

Functions that don't return a value shouldn't have a Returns section.

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

Show all issues

This seems fragile. I'm not wild about it. The idea is to have a version of command that didn't have a --help variant in it, right?
It's best when making a unit test helper function to not have to guess too much about the caller's intention. I think this should simply take another parameter specifying the RBTools subcommand name. subcommand_name should be fine. You can then just pass that into the output strings, and avoid the guessing here.

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

Show all issues

Rather than defining the variable, you can move the assertIn into those if statements.

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

Show all issues

This is only here for the execute command, right? In this case, let's only wrap as much as we need to in the try/except. We should only require the execute to be wrapped.

Change Summary:

More refactoring

Commit:

-	d06066f5d11f28a571fadd2fb44f6ee97af8a6cf
+	392c0509e6981b9680df340fa36434e5b89c305f

Diff:

Revision 9 (+80 -1)

Show changes

	rbtools/commands/main.py
	rbtools/commands/tests/test_main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

This looks great. I really appreciate the detailed description / testing don fields, and I love that we're adding tests for this. Thanks!

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (0143911)