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

fix(usage): clean up usage declarations #2821

Closed
wants to merge 8 commits into from
Closed

fix(usage): clean up usage declarations #2821

wants to merge 8 commits into from

Conversation

wraithgar
Copy link
Member

Small refactor of commands to allow usage to be more programmatically
generated, leading us in the direction of more tighly coupling each
command to the params it accepts.

Needs #2772 to land first before this is really reviewable.

@wraithgar wraithgar requested a review from a team as a code owner March 4, 2021 16:44
@darcyclarke darcyclarke added semver:patch semver patch level for changes Release 7.x work is associated with a specific npm 7 release Needs Review labels Mar 4, 2021
@wraithgar wraithgar force-pushed the gar/usage branch 2 times, most recently from ac488cb to f9fd164 Compare March 4, 2021 18:33
@wraithgar wraithgar force-pushed the gar/usage branch 3 times, most recently from cedb1e1 to 3018705 Compare March 4, 2021 22:26
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

Found a few pretty small bugs. I like where this is going!

lib/access.js Outdated Show resolved Hide resolved
'npm access edit [<package>]'
)
/* istanbul ignore next - see test/lib/load-all-commands.js */
static get usage () {
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉🎉🎉 💖

lib/base-command.js Outdated Show resolved Hide resolved
'--package=<pkg>[@<version>] -- <cmd> [args...]',
'-c \'<cmd> [args...]\'',
'--package=foo -c \'<cmd> [args...]\'',
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping the bit about running without --call or positional args to enter a subshell. Maybe could add something like:

  '(without -c or arguments to enter subshell)',

Or tack it onto the description?

Also the npx stuff is missing, but probably that should just be moved to bin/npx-cli.js anyway.

Copy link
Member Author

@wraithgar wraithgar Mar 6, 2021

Choose a reason for hiding this comment

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

This is usage, not the man page. This just shows the args you can use. Because it was free-form before it got a little muddied. If I ask for usage on command a, I don't really need to see how to use command b. If we want a "see also" we can talk about that but I really think that belongs in help

Copy link
Member Author

Choose a reason for hiding this comment

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

Having each flag describe what it does is definitely the direction this is moving us towards. For now we are separating our man page from basic usage. Next iteration we can define args instead of calling this "usage"

Usage is something that is built up from components i.e. command name, aliases, command description, args and their descriptions, etc.

lib/init.js Outdated Show resolved Hide resolved
lib/completion.js Outdated Show resolved Hide resolved
@wraithgar wraithgar force-pushed the gar/usage branch 2 times, most recently from 56698e5 to cc2faa4 Compare March 6, 2021 00:52
@wraithgar
Copy link
Member Author

@isaacs I tweaked the output to make it a little more nicely for things that don't have a description. I think we should find a "good enough" line here and then bikeshed the next phase.

(I still need to make the tests pass for this change on Monday)

@wraithgar wraithgar force-pushed the gar/usage branch 2 times, most recently from a9b6c58 to ea57efd Compare March 6, 2021 05:05
@darcyclarke darcyclarke added the release: next These items should be addressed in the next release label Mar 6, 2021
@isaacs isaacs self-requested a review March 9, 2021 16:47
All output that anything wants to make now goes through
`npm.output()`.  This is an incremental change getting us
closer to where we want to be with testing.

PR-URL: #2795
Credit: @wraithgar
Close: #2795
Reviewed-by: @ruyadorno, @isaacs
Small refactor of commands to allow usage to be more programmatically
generated, leading us in the direction of more tighly coupling each
command to the params it accepts.

PR-URL: #2821
Credit: @wraithgar
Close: #2821
Reviewed-by: @isaacs
This was referenced Mar 12, 2021
@wraithgar wraithgar deleted the gar/usage branch November 2, 2021 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants