-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
Add support to print help/usage of util.parseArgs #58875
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
base: main
Are you sure you want to change the base?
Add support to print help/usage of util.parseArgs #58875
Conversation
* // returns '--foo <arg> help text' | ||
*/ | ||
function formatHelpTextForPrint(longOption, optionConfig) { | ||
const layoutSpacing = 30; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found print has some logic related to layout spacing, maybe we can use something similar here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58875 +/- ##
==========================================
- Coverage 90.06% 90.06% -0.01%
==========================================
Files 645 645
Lines 189130 189190 +60
Branches 37094 37104 +10
==========================================
+ Hits 170339 170388 +49
+ Misses 11511 11507 -4
- Partials 7280 7295 +15
🚀 New features to boost your workflow:
|
help: 'My CLI Tool v1.0\n\nProcess files with various options.', | ||
}); | ||
|
||
if (result.printUsage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current implementation, printUsage
is always defined with this example code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printUsage
will be available on result
only if help config or help text on options are present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (result.printUsage) { | |
if (result.values.help) { |
doc/api/util.md
Outdated
options, | ||
help: 'My CLI Tool v1.0\n\nProcess files with various options.', | ||
enableHelpPrinting: true, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the enableHelpPrinting
here is a good idea. Instead, the return value of parseArgs
should just be capable of returning the serialized help text. Allowing for something like,
const result = parseArgs(...);
console.log(result.printUsage());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jasnell So we wouldn't have this flag or anything checking -h, --help
to print the help text internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have something, yes, but having these kinds of side effects on the constructor can be problematic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jasnell I made some changes so that we have the serialized help text available in printUsage
.
I still kept enableHelpPrinting
for now because with the -h, --help
check, I can't think of another way to do this control when displaying the help text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you key on the presence of help text, and --help
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not agreement on this point, I think. @ljharb wants parseArgs
to look for --help
and do the printing itself. I read @jasnell above and @shadowspawn here as preferring not to have that behavior, and instead just making it return the formatted text, with the user checking --help
themselves.
I am also in the camp of preferring not to do the automatic printing. Functions called called parse
should not also be implementing other behavior. Besides, there's just too many cases where you want don't want that behavior. For example, if you're building a CLI app with subcommands, you may be initially invoking parseArgs
just to get the tokens array, in which case you definitely don't want it adding any behavior other than parsing.
I could live with the automatic printing if it was off by default, but it's hardly any more work for the user to define and check for values.help
themselves, so personally I don't see much point. Either way, I think you'll need to get agreement on what behavior to have before this can go forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine with me if it doesn't do the automatic printing - in that case, the presence of the help text should be sufficient to cause it to be returned whether --help
is passed or not, and no extra config would be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bakkot Thanks for the mediation!
@ljharb just to double check, when you say:
the presence of the help text should be sufficient to cause it to be returned whether
--help
is passed or not, and no extra config would be needed.
Does it mean what we have in the serialized help text available as it is now in result.printUsage
, or something beyond that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean what we have in the serialized help text available as it is now
Yes, I think so.
So if the author passes in some help
strings, they get back something they can show the user themselves.
In general, what I'd expect is that |
@ljharb I'm not sure about printing the full help on validation failures. Once you have even three or four options that ends up being a lot of text, almost all of which is irrelevant to the actual error. Compare the output of I might be supportive of an option which makes validation failures just print the error and exit, instead of throwing a noisy error, but I don't think it makes sense to dump the full help text. |
That's a solid argument for allowing, but not defaulting, that behavior :-) |
I think that having commands print their full help text on argument validation failure is sufficiently unusual that it doesn't make sense to support it; I can't actually find any commands on my system that have that behavior, after trying a bunch at random. |
Interesting, I've seen that a lot in CLI tools on npm, albeit not in shell/OS ones. As long as the ability to trigger the help text exists, it can be worked around, so that's fine. |
doc/api/util.md
Outdated
@@ -1973,13 +1978,19 @@ changes: | |||
the built-in behavior, from adding additional checks through to reprocessing | |||
the tokens in different ways. | |||
**Default:** `false`. | |||
* `help` {string} General help text to display at the beginning of help output. | |||
* `enableHelpPrinting` {boolean} When `true`, if any options have help text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would you want the help text present, but --help
not to print the help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, the idea was to check -h --help
presence with enableHelpPrinting
to have that behaviour of printing + exit, If we don't have both of them present, the formatted string will only be returned in printUsage
with the general help
args plus the help texts for each option that were made available, does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really - the formatted string should always be returned with printUsage if help text is present, and --help
should just always work when help text is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would you want the help text present, but
--help
not to print the help?
When you are using a different option for displaying the help, such as with localised text.
The ability to specify a custom trigger option for the help would be full featured, but potentially excessive. I was thinking just the ability to get the help text returned allows the end-user to easily use their own option in the normal way and trigger the display of the help text themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user provided the help text - why would they need it back? Localization would be on their end, before invoking parseArgs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really - the formatted string should always be returned with printUsage if help text is present, and
--help
should just always work when help text is present.
Yes, I think I got confused in my explanation. Both features will only work if the help text is present, but for print+exit, we will have -h --help
also provided as args.
doc/api/util.md
Outdated
options, | ||
help: 'My CLI Tool v1.0\n\nProcess files with various options.', | ||
enableHelpPrinting: true, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you key on the presence of help text, and --help
?
6110e02
to
98c0d7e
Compare
const help = objectGetOwn(optionConfig, 'help'); | ||
|
||
let helpTextForPrint = ''; | ||
if (help) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that options without explicit help text are still included in the formatted help. This makes basic help simple. Add any help field, including describing the overall program, and you get formatted help returned with all the available options included.
Yes, some users will want "hidden" options. But I think that is uncommon, and more useful to make it easy to get help text for simple programs with (well named) options, without needing to describe each option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(--charlie
and --india
in the big example in the tests.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also a lot easier to filter out help text you don't want, than to recreate and format help text you do want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadowspawn @ljharb sounds good! I just pushed the changes related to this and removed the automatic printing, thanks for all the help so far!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining that supplying any help
text, whether the general help
or for an option, would trigger returning the formatted help in the results. The current implementation requires passing a general text
property. (And the documentation says that too, so self consistent.)
I don't feel strongly about this. See what others think.
If the help without general text was allowed it would look like:
% node index.cjs --help
-v, --verbose
-h, --help Prints command line options
--output <arg> Output directory
98c0d7e
to
a565569
Compare
help: 'My CLI Tool v1.0\n\nProcess files with various options.', | ||
}); | ||
|
||
if (result.printUsage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (result.printUsage) { | |
if (result.values.help) { |
} else if (type === 'boolean') { | ||
helpTextForPrint += ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these two lines do anything?
|
||
* Returns: {Object} The parsed command line arguments: | ||
* `values` {Object} A mapping of parsed option names with their {string} | ||
or {boolean} values. | ||
* `positionals` {string\[]} Positional arguments. | ||
* `tokens` {Object\[] | undefined} See [parseArgs tokens](#parseargs-tokens) | ||
section. Only returned if `config` includes `tokens: true`. | ||
* `printUsage` {string | undefined} Formatted help text for all options provided. Only included if general `help` text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some of the original discussion, printUsage
was a function and hence the name. It was suggested that simpler to just return a string, which is what you have done here. So I think the name could be changed.
* `printUsage` {string | undefined} Formatted help text for all options provided. Only included if general `help` text | |
* `usage` {string | undefined} Formatted help text for all options provided. Only included if general `help` text |
|
||
`parseArgs` supports automatic formatted help text generation for command-line options. To use this feature, provide | ||
general help text using the `help` config property, and also | ||
add a `help` property to each option can be optionally included. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a `help` property to each option can be optionally included. | |
a `help` property to each option can be optionally included. |
); | ||
}); | ||
|
||
test('when help arg with help value for lone short option is added, then add help text', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the test seems to be describing the arguments to be parsed rather than the interesting variations of the formatted help. I might be misunderstanding the intent.
Perhaps:
test('when help arg with help value for lone short option is added, then add help text', () => { | |
test('when option has short and long flags, then both appear in usage', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was lone
perhaps referring to there only being one option? Single option is fine as testing one thing at a time is good for unit tests.
I think things of interest to test are:
- option with no short flag
- option with long and short flag
- string option
- boolean option
- option without help text
- long option that causes wrap
You are testing all of these (so good coverage), but the test names seem to be describing the supplied args
rather than the supplied options
.
Issue: 56184
Add support to print help/usage of
util.parseArgs
This PR adds help text functionality to
util.parseArgs
, allowing developers to easily generate and display help information for their CLI applications.Checklist
printUsage
added to return object