-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
feat(help): refactor npm help/help-search #2859
Conversation
0c9d405
to
bf6c3a8
Compare
lib/access.js
Outdated
@@ -20,6 +20,10 @@ const subcommands = [ | |||
] | |||
|
|||
class Access extends BaseCommand { | |||
static get description () { | |||
return 'Set access level on published packages' |
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.
These all now should match the descriptions in the docs for all the commands, which means we are one step away from being able to programmatically generate everything before the longhand "Description" sections of those docs.
2e175cf
to
7f6195e
Compare
lib/docs.js
Outdated
constructor (npm) { | ||
this.npm = npm | ||
const BaseCommand = require('./base-command.js') | ||
class Docs extends BaseCommand { |
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 added a test to test/lib/load-all-commands.js
that caught this bug. Yay.
lib/docs.js
Outdated
} | ||
|
||
/* istanbul ignore next - see test/lib/load-all-commands.js */ | ||
get usage () { | ||
return usageUtil('docs', 'npm docs [<pkgname> [<pkgname> ...]]') | ||
static get 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.
I added a test to test/lib/load-all-commands.js that caught this bug. Yay.
lib/help-search.js
Outdated
@@ -26,28 +27,17 @@ class HelpSearch extends BaseCommand { | |||
|
|||
async helpSearch (args) { | |||
if (!args.length) | |||
throw this.usage | |||
return this.npm.output(this.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.
Now that this isn't pulling triple duty it can just work like everything else and return usage on no params
lib/help-search.js
Outdated
const formatted = this.formatResults(args, results) | ||
if (!formatted.trim()) | ||
npmUsage(this.npm, false) | ||
else { | ||
this.npm.output(`No matches in help for: ${args.join(' ')}\n`) |
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.
Now that this isn't the usage output for the cli itself we can act in isolation and not have to return usage here.
lib/help.js
Outdated
|
||
let pref = [1, 5, 7] |
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 old method would treat the number in the args as a suggestion, there is no need to search everywhere if we are explicitly asking for a given man number. This was the source of most of the streamlining here.
7f6195e
to
4dc3de1
Compare
4dc3de1
to
a60c5a8
Compare
a60c5a8
to
41facf6
Compare
Lots of dead code removed thanks to streamlining of logic.
npm help
npm <command>
andnpm help-search
are all now separatedconcerns, handling their own use cases.
help
callshelp-search
as alast resort, but
npm <command>
no longer tries to wind its way throughto
help-search
just to get the basic npm usage displayed.The
did you mean
output has been expanded. It now always suggests toplevel commands, scripts, and bins, and suggests them in the way they
should be called.
References
Closes npm/statusboard#277