Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to override help and version help message #963

Conversation

idmontie
Copy link
Contributor

@idmontie idmontie commented May 16, 2019

Close #47.

Add override for the help message by calling program.help(message).

Add override for the version message by calling
program.version(version, flag, message);

@shadowspawn
Copy link
Collaborator

shadowspawn commented May 17, 2019

Did you see #870, which does the same thing? I'll give them a week to make contact, otherwise I'll work with you.

Some of my feedback will be same as in #870 as it happens! I like the version changes, but not the help changes. In the longer term we might want something like .help(helpFlags, helpDescription, helpOptions) and I don't want partial solutions that will get in way of a more complete solution.

edit: pull request was updated to match this approach

If we go ahead, we'll want JSDoc and TypeScript updates for the .version function.

Thank you for your contributions.

@idmontie
Copy link
Contributor Author

@shadowspawn I changed my code to support .help(helpFlags, helpDescription). I didn't add helpOptions since I don't know what those options would be or what they would do. But it should be easy to add them later.

I changed the hardcoded help and h references throughout the index and added some examples and a test to check that changing the flags works as intended.

I added typings as well.

Let me know if there are any other changes I should make.

@shadowspawn
Copy link
Collaborator

shadowspawn commented May 18, 2019

Oh, exciting! I was not expecting you to run with the full help customisation.

I gave .help as example not paying attention to existing call. I'll need to think if tidy enough to overload that, but does avoid adding a new routine which could conflict with existing client use, so there is an advantage to overload too.

I'll let you know if I have any more requests or questions.

@shadowspawn shadowspawn self-requested a review May 18, 2019 08:26
@idmontie
Copy link
Contributor Author

@shadowspawn Did you have time to think about whether overriding the .help function was the best way forward?

@shadowspawn
Copy link
Collaborator

I'm still ok with .help, so don't change code @idmontie.

The other names I came up with were .autoHelp and .helpOption, but I currently prefer the simplicity of .help. Comments welcome from any readers.

@shadowspawn
Copy link
Collaborator

(In top two of my PR queue. Hopefully take another look this week.)

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, lots of feedback! But looking pretty good.

I added a bunch of smaller minor suggested changes on lines.

The bigger one is ideally want subcommand to get the custom flags too. (I have not looked into how to sensibly do that, possibly checking parent when created?)

i.e. currently get mixture of old and new help flags when have a sub-command

program
   .help("-i, --ihelp", "foo foo") ;

program
  .command("child")
  .option("--gender", "specific gender of child")
  .action((cmd) => {
    console.log("Childsubcommand...");
  });

Get the new flags for global help (as expected), but default flags for subcommand help:

$ node index.js -i
Usage: index [options] [command]

Options:
  --global         A global option
  -i, --ihelp      foo foo

Commands:
  child [options]
Bagheera:963 john$ node index.js child
Childsubcommand...
Bagheera:963 john$ node index.js child -i
error: unknown option `-i'
Bagheera:963 john$ node index.js child -h
Usage: child [options]

Options:
  --gender    choose sex of child
  -h, --help  output usage information

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
typings/index.d.ts Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

I looked briefly at the addImplicitHelpCommand and help foo behaviour but willing to leave that for future!

@shadowspawn
Copy link
Collaborator

(Feel free to ask if you have any questions about my comments, or what I was thinking.)

@shadowspawn
Copy link
Collaborator

I should say, I am planning to investigate pattern for inheritance of settings from "program" down into commands myself anyway for #951. So don't feel you have to solve that yourself, feel free to wait for a pattern for that part.

@idmontie
Copy link
Contributor Author

idmontie commented Jun 4, 2019

Sorry for disappearing for a bit. I'm hoping to have have time this week to tackle your comments.

@shadowspawn
Copy link
Collaborator

No worries, thanks for update. I usually give people a month before taking other action. :-)

@shadowspawn
Copy link
Collaborator

Thanks for updates, I am excited to check them out. Might be a few days before I have enough time.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shadowspawn
Copy link
Collaborator

Adds custom version description, custom help flags, custom help description. (Nice!)

This is technically a minor version as new features that should be backwards compatible, but changes enough that we might be better to be cautious and roll it into v3 @abetomo ?

@abetomo
Copy link
Collaborator

abetomo commented Jun 9, 2019

I think that it is all right with the v2 release.

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@shadowspawn shadowspawn added this to the v2.21.0 milestone Jun 10, 2019
@shadowspawn shadowspawn self-assigned this Jun 16, 2019
@shadowspawn
Copy link
Collaborator

I am wondering again whether overloading .help might not be best approach and cause confusion in future, because the IDE autocompletion will be complicated with three ways of calling .help. In TypeScript we could improve this by having separate function overloads, but with JSDoc I don't think that is an option.

I came back to this because of a fresh report over confusion due to command description triggering git-style executable commands, reminding me that compact and "clever" can be too subtle and cause confusion for years!

The two names I had come up with previously were .autoHelp and .helpOption. Comments and alternatives welcome from any readers. .helpOption aligns with .option and flags and description, while autoHelp emphasizes that it is a bit special.

No code changes wanted at this stage @idmontie, but I am thinking again about what will be best in longterm.

@shadowspawn shadowspawn modified the milestones: v2.21.0, v3.0.0 Jun 22, 2019
@shadowspawn
Copy link
Collaborator

This is a strong candidate for v3 as has passed two code reviews. I am leaning towards naming the custom help routine something different rather than overloading though. If I decide I want to go with that, would you like to do the work @idmontie ? (I am willing to make the changes, in which case I would invite you to review it.)

@idmontie
Copy link
Contributor Author

@shadowspawn I can do that change if you provide the function name 👍

@shadowspawn
Copy link
Collaborator

Thanks @idmontie. Settled on .helpOption. e.g.

program
   .helpOption('-c, --HELP', 'custom help message')

Close tj#47.

Add override for the help message by calling program.help(message).

Add override for the version message by calling
program.version(version, flag, message);
Ivan and others added 5 commits June 25, 2019 11:17
Sub-commands now inherit the help flags of their parent. These
can be overriden by calling ".help" on the sub-commands.

Added a test to cover these cases. Updated the example to also
demonstrate this feature.

Improved documentation on help and outputHelp functions for
both JS and TypeScript.
@idmontie idmontie force-pushed the feature/issue-47-add-version-help-message-override branch from 1b58cf3 to 14f54c5 Compare June 25, 2019 18:17
@shadowspawn
Copy link
Collaborator

shadowspawn commented Jun 26, 2019

Thanks @idmontie. I am going to start merging to release/v3 branch.

There are a couple of left-over bits in typings/index.d.ts from the earlier overload approach, and I will fix those.

Thank you for your contributions.

shadowspawn pushed a commit that referenced this pull request Jun 26, 2019
Minor edits from John Gee to typings added during squash.
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 26, 2019
@shadowspawn
Copy link
Collaborator

Merged into v3 and will be included in that release. Closing to make it clear that should not be merged into master.

FYI Ivan, I did a squash during manual merging, fixed the typings, and made you the author of the commit.

Thank you for your contributions.

shadowspawn added a commit that referenced this pull request Jun 26, 2019
@shadowspawn
Copy link
Collaborator

Available now as a prerelease. See #1001

@shadowspawn
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A way to override the version and help message
3 participants