[ts-command-line] Update to argparse@2.0.1 and default allow_abbrev to false
#4403
Conversation
D4N14L
requested review from
iclanton,
octogonz,
apostolisms,
dmichon-msft and
patmill
as code owners
D4N14L
changed the title
argparse@2.0.1 and default allow_abbrev to false
|
|
||
| if (parameter.undocumentedSynonyms?.length) { | ||
| argumentGroup.addArgument(parameter.undocumentedSynonyms, { | ||
| for (const undocumentedSynonym of parameter.undocumentedSynonyms || []) { |
There was a problem hiding this comment.
Wrapping in the if is marginally more efficient
|
|
||
| a longer description | ||
|
|
||
| Optional arguments: | ||
| -h, --help Show this help message and exit. | ||
| optional arguments: |
There was a problem hiding this comment.
This inconsistent formatting looks bad; if the base is changing should we change to match?
There was a problem hiding this comment.
I don't think so. Beginning sentences with a lower case letter looks objectively worse. I checked a few other popular CLI tools (tsc, eslint, PowerShell, gcc) and their --help always uses proper English capitalization. Doing so also encourages grammatically complete sentences, which may inspire better documentation. Notably python --help does show unprofessional lowercase capitalization, however Python's own pip --help uses caps like all the other examples.
I believe this behavior is customizable by implementing our own argparse help formatter.
There was a problem hiding this comment.
The API allows us to suppress the default --help parameter and define it ourselves.
I created an issue nodeca/argparse#175 requesting a more general solution, however the NPM package hasn't been updated in 3 years, so we'll see if they reply. 😄
|
Closed in favor of #4405 which implements the main goal of this PR, without wholesale disabling |
Summary
This PR bumps the version of argparse consumed to
2.0.1. It also defaultsallow_abbrevtofalsewhen creating the underlying argument parser, to avoid issues with unexplicit parameter usage (ex.--debugmatching--debug-heft-reporterwhich comes from the@rushstack/heft-jest-pluginpackage). More documentation on the parameter can be found here: https://docs.python.org/3/library/argparse.html#allow-abbrevDetails
API usage across the entire argparse surface had to be updated, since the underlying library now uses
snake_casing, but behaviour remains the same.How it was tested
All unit tests passed for ts-command-line, new test added for abbreviation scenario and confirmed the test failed without setting
allow_abbrevtofalse.Impacted documentation
None. This was undocumented and unintended behaviour.