Fix help so it displays per grouping

[jdonaghue]

Shows sub commands and the default command's options as well as global options

Type: bug

Description:

Fix help display so that it displays options per grouping

Related Issue: #58

Please review this checklist before submitting your PR:

• [x] There is a related issue
• [x] All contributors have signed a CLA
• [x] All code matches the style guide
• [x] The code passes the CI tests
• [x] Unit or Functional tests are included in the PR
• [x] The PR increases or maintains the overall unit test coverage percentage
• [x] The code is ready to be merged

[codecov-io]

Current coverage is 99.16% (diff: 100%)

Merging #85 into master will increase coverage by <.01%

@@             master        #85   diff @@
==========================================
  Files            11         11          
  Lines           237        239     +2   
  Methods           0          0          
  Messages          0          0          
  Branches         20         20          
==========================================
+ Hits            235        237     +2   
  Misses            0          0          
  Partials          2          2          

Powered by Codecov. Last update 96216a6...ac5313c

[tomdye]

This needs a test for the bug it's trying to fix.

[tomdye]

@jdonaghue have you been able to test this in the case that we have two commands for a given group? Please can you attach some screenshots to the PR of the output you are getting for a given input (as per previous CLI PR's) so than we can clarify the result.

[jdonaghue]

@Tomdye I am working on the tests now and will add the screenshots shortly after that. Good suggestions, thanks!

[tomdye]

Im not sure the output is quite right @jdonaghue.

Given a situation where we had two build commands registered, webpack and rjs. When typing dojo build -h I would expect to see:

Commands:
   webpack
   rjs

Options:
   -h, --help   Show help

Only when typing dojo build webpack -h would I expect to see the options for webpack build showing:

Options:
   -w, --watch   Run as watch
   -f, --foo   Foo option
   -h, --help   Show help

Finally, if there is only one command registered under build, we would show the same output as above.

@agubler can you confirm if this is what you had in mind with the issue you raised?

[jdonaghue]

@Tomdye I can certainly make it behave that way but not if we want to allow for a default command. The default command will run but not accept any options in that scenario. Currently, when the build command is registered we set the default commands options (for dojo in the example above). If we do this then those options will always display somewhere when dojo build -h is typed in. Either under the Options section or beneath a dojo section. Prior to this fix they were showing up under the Options section. This is because we are associating those options with the build command (as well as the dojo command). In order to get what you described above we would need to ditch the concept of a default command (while working with yargs anyway).

[jdonaghue]

@Tomdye we could do a little conditional logic and if there is only one command not associate a group with the options that would allow for the last scenario that you mentioned to work that way. But the 1st and 2nd scenarios will have to behave the way I implemented it if we are to keep the concept of a default command and not want only the default commands options to show when dojo build -h is entered.

[tomdye]

re. the default command. At the moment it is just the first command loaded for a given group, so it's not really a thing. We still need to work out if it's even something we will support when a group has multiple commands available and how we will deliver that. So for the time being I would think that given that information the output mentioned above should be possible.

[jdonaghue]

OK cool if we don't need to support a default command then yes that format is possible. I will make the changes necessary to accommodate that. Thanks!

[jdonaghue]

I have made the desired changes and here is what the help output looks like now:

[tomdye]

This still needs defaultcommand to be re-instated

[agubler]

@jdonaghue are you still working on reinstating default command functionality?

[jdonaghue]

Ok this is the best that I could do for the default command:

Here is for dojo:

Here is webpack:

The reason for the default command help options looking the way they do is that command option registration occurs prior to command execution. So at the time when registering options we don't know how the cli is being called making it impossible to conditionally register option based on call signature. This combined with the fact that you must register options upfront when using yargs in order to allow them to be entered, restrict that only they may be entered and then show them in help makes it impossible to not show the default commands options when running help on that command (eg dojo build -h).

So we have a choice between two scenarios:

1. Just show the default options under the Options section (not indicating which command they are for.
2. Group the options under a descriptive name (as is done in the picture above) and showing them with some context.

I think what we would ideally want is to tell yargs about those options but not have them displayed in this scenario, but that is (as far as I can tell with the current version of yargs@4.8.1) impossible.

[tomdye]

PR merge is dependant on: https://github.com/dojo/cli-create-app/pull/29 and https://github.com/dojo/cli-build/pull/26 both of which currently have conflicts that need resolving.

[jdonaghue]

@tomdye https://github.com/dojo/cli-create-app/pull/29 and https://github.com/dojo/cli-build/pull/26 are both failing only because this PR hasn't landed yet. Since they both depend on changes to interfaces in this PR but are pulling `. Should I just update those two PRs to point to the most recent commit from this PR? Or is it better to land this and publish it first and then update those two PR's with the newly published version?

[agubler]

@jdonaghue we shall merge and publish these changes then update the dependant PRs to point to the updated CLI version.