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

Review Request #10856 — Created Jan. 24, 2020 and submitted

Information

RBTools
release-2.0.x
392c050...

Reviewers

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.

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

Description From Last Updated

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

chipx86chipx86

Can you wrap the change description at 79 columns? It would also be nice to use backticks whenever you're showing …

chipx86chipx86

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (94 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (88 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (88 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

I think this docstring and file could be named in a more descriptive way eg. Tests for RBTools help command …

hxqlinhxqlin

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

hxqlinhxqlin

I think this could be better named as test_help_command. Ordering it that way would make it more consistent with the …

hxqlinhxqlin

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

hxqlinhxqlin

This snippet feels like it could also be a function since you use it a few times. Also, maybe command_string …

hxqlinhxqlin

Same as above

hxqlinhxqlin

Minor but maybe naming test_help to be test_command would be better here to be more consistent with your first test …

hxqlinhxqlin

Same as above

hxqlinhxqlin

Same as above

hxqlinhxqlin

Same as above

hxqlinhxqlin

Since you're testing help already, it might be a good idea to add tests for when there are more than …

hxqlinhxqlin

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

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

chipx86chipx86

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

chipx86chipx86

Blank line between blocks/statements. Same below.

chipx86chipx86

Docstring summaries are limited to one line. Same below.

chipx86chipx86

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

chipx86chipx86

"Returns" blocks are in the form of: Returns: type: description So probably unicode for the type.

chipx86chipx86

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

chipx86chipx86

Blank line between each argument.

chipx86chipx86

bool

chipx86chipx86

bool

chipx86chipx86

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

I've been thinking about these code blocks, which are almost universal in your code. I think what would be better …

chipx86chipx86

How about making this one statement? Same below.

chipx86chipx86

This could be one line now. Same with ones below.

chipx86chipx86

Sorry, I wasn't clear in my description above. We'd want a blank line when separating blocks on the same indentation …

chipx86chipx86

This can be one statement.

chipx86chipx86

Here, and below, it'll be a lot easier to debug if we're passing in the command directly to _check_help_output. The …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This seems fragile. I'm not wild about it. The idea is to have a version of command that didn't have …

chipx86chipx86

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

chipx86chipx86

This is only here for the execute command, right? In this case, let's only wrap as much as we need …

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

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

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

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

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

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

    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

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

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

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

    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.

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

    Same as above

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

    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)

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

    Same as above

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

    Same as above

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

    Same as above

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

    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

  13. 
      
kpatenio
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

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

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

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

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

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

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

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

    Same with docstrings below.

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

    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.

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

    Blank line between blocks/statements.

    Same below.

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

    Docstring summaries are limited to one line.

    Same below.

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

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

    Same below.

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

    "Returns" blocks are in the form of:

    Returns:
        type:
        description
    

    So probably unicode for the type.

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

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

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

    Blank line between each argument.

  12. 
      
kpatenio
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
kpatenio
chipx86
  1. 
      
    1. 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!

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

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

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

    How about making this one statement?

    Same below.

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

    This could be one line now.

    Same with ones below.

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

    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:
            ...
    
  6. 
      
kpatenio
bui1
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/tests/test_main.py (Diff revision 8)
     
     
     

    This can be one statement.

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

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

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

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

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

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

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

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

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

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

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

    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.

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

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

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

    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.

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

  2. 
      
chipx86
  1. Ship It!
  2. 
      
kpatenio
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (0143911)
Loading...