Improve help support for custom commands and aliases

Review Request #11552 — Created March 23, 2021 and updated

mblythe
RBTools
master
rbtools
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 tested rbt help <cmd> and rbt <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
Improve help support for custom commands and aliases
Description From Last Updated

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

We prefer imports to be listed in alphabetical order.

daviddavid

I think I'd prefer if the subprocess.call() and sys.exit() are separate statements.

daviddavid

Instead of doing a whole bunch of list operations, then casting to set, then back to list, let's keep this ...

daviddavid

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

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

flake8

mblythe
Review request changed

Commits:

Summary
-
Improve help support for custom commands and aliases
+
Improve help support for custom commands and aliases

Diff:

Revision 2 (+64 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mblythe
Review request changed

Commits:

Summary
-
Improve help support for custom commands and aliases
+
Improve help support for custom commands and aliases

Diff:

Revision 3 (+64 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mblythe
david
  1. 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?

  2. rbtools/commands/main.py (Diff revision 4)
     
     

    We prefer imports to be listed in alphabetical order.

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

    I think I'd prefer if the subprocess.call() and sys.exit() are separate statements.

  4. 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)
    
  5. 
      
mblythe
  1. 
      
  2. 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?

    1. Yes, that would be good.

  3. 
      
mblythe
mblythe
Review request changed

Change Summary:

Incorporating recommended changes

Commits:

Summary
-
Improve help support for custom commands and aliases
+
Improve help support for custom commands and aliases

Diff:

Revision 5 (+90 -14)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mblythe
Review request changed

Change Summary:

fixing flake8 errors

Commits:

Summary
-
Improve help support for custom commands and aliases
+
Improve help support for custom commands and aliases

Diff:

Revision 6 (+90 -14)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mblythe
Review request changed

Change Summary:

fixing flake8 errors...again.  It would be helpful if the error stated the expected intendation so I don't have to guess.

Commits:

Summary
-
Improve help support for custom commands and aliases
+
Improve help support for custom commands and aliases

Diff:

Revision 7 (+90 -14)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...