flake8
-
rbtools/commands/main.py (Diff revision 1) -
-
Review Request #11552 — Created March 23, 2021 and submitted
rbtools supports adding custom commands by either finding a corresponding executable on $PATH beginning with 'rbt-' or by setting an alias. However, these custom commands do not appear when running 'rbt help'. Also, getting command-specific help does not work - neither 'rbt help new-command' nor 'rbt new-command --help'. This change fixes both of these shortcomings.
Tested locally on my own ReviewBoard instance with both commands on $PATH and aliases. I saw both listed in
rbt help
. I also testedrbt help <cmd>
andrbt <cmd> --help
for both commands on $PATH and aliases, and usage (or the alias string) was printed as expected.I did not run the automated rbtools test suite. \<excuses> It looked somewhat involved to get it set up (I'm a python newbie), and I wasn't sure whether the automated testing would actually hit my new code, anyway \</excuses>
Summary | ID |
---|---|
4d5771187e9822f9baa2fb305658c1e93d6fa4b8 |
Description | From | Last Updated |
---|---|---|
E501 line too long (80 > 79 characters) |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
We prefer imports to be listed in alphabetical order. |
david | |
I think I'd prefer if the subprocess.call() and sys.exit() are separate statements. |
david | |
Instead of doing a whole bunch of list operations, then casting to set, then back to list, let's keep this … |
david | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
One last thing I noticed--since we have the commands as a set, we should be able to filter common_commands out … |
david |
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+64 -2) |
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+64 -2) |
sorry for the multiple diff uploads - it took me a while to realize that I was fixing the issues in a different repo than the one I was submitting from!
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+64 -2) |
Thanks for the contribution! I've got a few comments mostly related to style.
Can you also detail what testing you've done for this change?
rbtools/commands/main.py (Diff revision 4) |
---|
I think I'd prefer if the
subprocess.call()
andsys.exit()
are separate statements.
rbtools/commands/main.py (Diff revision 4) |
---|
Instead of doing a whole bunch of list operations, then casting to set, then back to list, let's keep this all as a set until the end. We also like to have blank lines around indented blocks. Perhaps something like this?
commands = {entrypoint.name for entrypoint in entrypoints} for path_dir in os.environ.get('PATH').split(':'): path_prefix = '%s/%s-' % (path_dir, RB_MAIN) for cmd in glob.glob(path_prefix + '*'): commands.add(cmd.replace(path_prefix, '')) aliases = load_config.get('ALIASES', {}) commands |= aliases.keys() commands = list(commands)
rbtools/commands/main.py (Diff revision 4) |
---|
Regarding separating the sys.exit and subprocess.call - I was copying the style from here. Would you like this one separated as well?
Updated 'testing done'
Testing Done: |
|
---|
Incorporating recommended changes
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+90 -14) |
fixing flake8 errors
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+90 -14) |
fixing flake8 errors...again. It would be helpful if the error stated the expected intendation so I don't have to guess.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+90 -14) |
This is looking pretty solid!
rbtools/commands/main.py (Diff revision 7) |
---|
One last thing I noticed--since we have the commands as a set, we should be able to filter
common_commands
out of it ahead of time. We probably can also skip the cast tolist
, sincesorted
should be able to handle it as-is:common_commands = ['post', 'patch', 'close', 'diff'] other_commands = commands - set(common_commands) print('\nThe most commonly used commands are:') for command in common_commands: print(' %s' % command) print ('\nOther commands:') for command in sorted(other_commands): print(' %s' % command)
Including suggestions from latest review
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+94 -20) |
rebased change to up-to-date master branch
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+94 -20) |