From 22230ef3dd590def31c274b3412106b4cfbd212f Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Wed, 3 Nov 2021 14:53:40 -0700 Subject: [PATCH] fix: make prefixed usage errors more consistent PR-URL: https://github.com/npm/cli/pull/3987 Credit: @lukekarrys Close: #3987 Reviewed-by: @wraithgar --- lib/base-command.js | 12 ++++-------- lib/commands/cache.js | 9 ++------- lib/commands/diff.js | 16 ++++++---------- lib/commands/edit.js | 2 +- lib/commands/exec.js | 2 +- lib/commands/explore.js | 4 ++-- lib/commands/help-search.js | 2 +- lib/commands/hook.js | 2 +- lib/commands/pkg.js | 10 ++-------- lib/commands/profile.js | 2 +- lib/commands/star.js | 2 +- lib/commands/team.js | 2 +- packages/libnpmdiff/index.js | 4 ++++ test/lib/commands/cache.js | 10 +++++----- test/lib/commands/help-search.js | 3 +-- test/lib/commands/pkg.js | 10 +++++----- 16 files changed, 38 insertions(+), 54 deletions(-) diff --git a/lib/base-command.js b/lib/base-command.js index 9657110f83291..c5fce6fd8d0bd 100644 --- a/lib/base-command.js +++ b/lib/base-command.js @@ -53,14 +53,10 @@ class BaseCommand { return results } - usageError (msg) { - if (!msg) { - return Object.assign(new Error(`\nUsage: ${this.usage}`), { - code: 'EUSAGE', - }) - } - - return Object.assign(new Error(`\nUsage: ${msg}\n\n${this.usage}`), { + usageError (prefix = '') { + if (prefix) + prefix += '\n\n' + return Object.assign(new Error(`\nUsage: ${prefix}${this.usage}`), { code: 'EUSAGE', }) } diff --git a/lib/commands/cache.js b/lib/commands/cache.js index 43f52e4e95e68..b4a932d1bec03 100644 --- a/lib/commands/cache.js +++ b/lib/commands/cache.js @@ -116,7 +116,7 @@ class Cache extends BaseCommand { case 'ls': return await this.ls(args) default: - throw Object.assign(new Error(this.usage), { code: 'EUSAGE' }) + throw this.usageError() } } @@ -161,14 +161,9 @@ class Cache extends BaseCommand { // npm cache add ... // npm cache add ... async add (args) { - const usage = 'Usage:\n' + - ' npm cache add ...\n' + - ' npm cache add @...\n' + - ' npm cache add ...\n' + - ' npm cache add ...\n' log.silly('cache add', 'args', args) if (args.length === 0) - throw Object.assign(new Error(usage), { code: 'EUSAGE' }) + throw this.usageError('First argument to `add` is required') return Promise.all(args.map(spec => { log.silly('cache add', 'spec', spec) diff --git a/lib/commands/diff.js b/lib/commands/diff.js index da9b9f5d2c655..67d0d15058534 100644 --- a/lib/commands/diff.js +++ b/lib/commands/diff.js @@ -49,12 +49,8 @@ class Diff extends BaseCommand { async exec (args) { const specs = this.npm.config.get('diff').filter(d => d) - if (specs.length > 2) { - throw new TypeError( - 'Can\'t use more than two --diff arguments.\n\n' + - `Usage:\n${this.usage}` - ) - } + if (specs.length > 2) + throw this.usageError(`Can't use more than two --diff arguments.`) // execWorkspaces may have set this already if (!this.prefix) @@ -101,7 +97,7 @@ class Diff extends BaseCommand { } if (!name) - throw this.usageError('Needs multiple arguments to compare or run from a project dir.\n') + throw this.usageError('Needs multiple arguments to compare or run from a project dir.') return name } @@ -133,7 +129,7 @@ class Diff extends BaseCommand { noPackageJson = true } - const missingPackageJson = this.usageError('Needs multiple arguments to compare or run from a project dir.\n') + const missingPackageJson = this.usageError('Needs multiple arguments to compare or run from a project dir.') // using a valid semver range, that means it should just diff // the cwd against a published version to the registry using the @@ -222,7 +218,7 @@ class Diff extends BaseCommand { `file:${this.prefix}`, ] } else - throw this.usageError(`Spec type ${spec.type} not supported.\n`) + throw this.usageError(`Spec type ${spec.type} not supported.`) } async convertVersionsToSpecs ([a, b]) { @@ -239,7 +235,7 @@ class Diff extends BaseCommand { } if (!pkgName) - throw this.usageError('Needs to be run from a project dir in order to diff two versions.\n') + throw this.usageError('Needs to be run from a project dir in order to diff two versions.') return [`${pkgName}@${a}`, `${pkgName}@${b}`] } diff --git a/lib/commands/edit.js b/lib/commands/edit.js index db9be4a267bfa..4f0af6e8333e6 100644 --- a/lib/commands/edit.js +++ b/lib/commands/edit.js @@ -35,7 +35,7 @@ class Edit extends BaseCommand { async exec (args) { if (args.length !== 1) - throw new Error(this.usage) + throw this.usageError() const path = splitPackageNames(args[0]) const dir = resolve(this.npm.dir, path) diff --git a/lib/commands/exec.js b/lib/commands/exec.js index 8f7f3c3e58bfd..ffe72ccb4c862 100644 --- a/lib/commands/exec.js +++ b/lib/commands/exec.js @@ -80,7 +80,7 @@ class Exec extends BaseCommand { const yes = this.npm.config.get('yes') if (call && _args.length) - throw this.usage + throw this.usageError() return libexec({ ...flatOptions, diff --git a/lib/commands/explore.js b/lib/commands/explore.js index 8ff88ddf67a23..81a71f86abace 100644 --- a/lib/commands/explore.js +++ b/lib/commands/explore.js @@ -34,14 +34,14 @@ class Explore extends BaseCommand { async exec (args) { if (args.length < 1 || !args[0]) - throw this.usage + throw this.usageError() const pkgname = args.shift() // detect and prevent any .. shenanigans const path = join(this.npm.dir, join('/', pkgname)) if (relative(path, this.npm.dir) === '') - throw this.usage + throw this.usageError() // run as if running a script named '_explore', which we set to either // the set of arguments, or the shell config, and let @npmcli/run-script diff --git a/lib/commands/help-search.js b/lib/commands/help-search.js index 4ab4a0896af44..a179939abf673 100644 --- a/lib/commands/help-search.js +++ b/lib/commands/help-search.js @@ -28,7 +28,7 @@ class HelpSearch extends BaseCommand { async exec (args) { if (!args.length) - return this.npm.output(this.usage) + throw this.usageError() const docPath = path.resolve(__dirname, '..', '..', 'docs/content') const files = await glob(`${docPath}/*/*.md`) diff --git a/lib/commands/hook.js b/lib/commands/hook.js index 96b6d96264ad3..7b2deff22940b 100644 --- a/lib/commands/hook.js +++ b/lib/commands/hook.js @@ -43,7 +43,7 @@ class Hook extends BaseCommand { case 'up': return this.update(args[1], args[2], args[3], opts) default: - throw this.usage + throw this.usageError() } }) } diff --git a/lib/commands/pkg.js b/lib/commands/pkg.js index c9b8f7d40d047..1fa2c3bc5777b 100644 --- a/lib/commands/pkg.js +++ b/lib/commands/pkg.js @@ -96,10 +96,7 @@ class Pkg extends BaseCommand { async set (args) { const setError = () => - Object.assign( - new TypeError('npm pkg set expects a key=value pair of args.'), - { code: 'EPKGSET' } - ) + this.usageError('npm pkg set expects a key=value pair of args.') if (!args.length) throw setError() @@ -123,10 +120,7 @@ class Pkg extends BaseCommand { async delete (args) { const setError = () => - Object.assign( - new TypeError('npm pkg delete expects key args.'), - { code: 'EPKGDELETE' } - ) + this.usageError('npm pkg delete expects key args.') if (!args.length) throw setError() diff --git a/lib/commands/profile.js b/lib/commands/profile.js index caab13d782fcc..abfe5edd7a9d7 100644 --- a/lib/commands/profile.js +++ b/lib/commands/profile.js @@ -90,7 +90,7 @@ class Profile extends BaseCommand { async exec (args) { if (args.length === 0) - throw new Error(this.usage) + throw this.usageError() log.gauge.show('profile') diff --git a/lib/commands/star.js b/lib/commands/star.js index 3e5b0fc620830..36003a02004be 100644 --- a/lib/commands/star.js +++ b/lib/commands/star.js @@ -30,7 +30,7 @@ class Star extends BaseCommand { async exec (args) { if (!args.length) - throw new Error(this.usage) + throw this.usageError() // if we're unstarring, then show an empty star image // otherwise, show the full star image diff --git a/lib/commands/team.js b/lib/commands/team.js index 11a7deb522b3a..b337a7536051f 100644 --- a/lib/commands/team.js +++ b/lib/commands/team.js @@ -68,7 +68,7 @@ class Team extends BaseCommand { return this.listTeams(entity, opts) } default: - throw this.usage + throw this.usageError() } }) } diff --git a/packages/libnpmdiff/index.js b/packages/libnpmdiff/index.js index 73dc3ee64e3ce..2d074c3d1bb67 100644 --- a/packages/libnpmdiff/index.js +++ b/packages/libnpmdiff/index.js @@ -4,6 +4,10 @@ const formatDiff = require('./lib/format-diff.js') const getTarball = require('./lib/tarball.js') const untar = require('./lib/untar.js') +// TODO: we test this condition in the diff command +// so this error probably doesnt need to be here. Or +// if it does we should figure out a standard code +// so we can catch it in the cli and display it consistently const argsError = () => Object.assign( new TypeError('libnpmdiff needs two arguments to compare'), diff --git a/test/lib/commands/cache.js b/test/lib/commands/cache.js index c12318f4e579b..168cfce6db3e7 100644 --- a/test/lib/commands/cache.js +++ b/test/lib/commands/cache.js @@ -3,8 +3,6 @@ const { fake: mockNpm } = require('../../fixtures/mock-npm.js') const path = require('path') const npa = require('npm-package-arg') -const usageUtil = () => 'usage instructions' - let outputOutput = [] let rimrafPath = '' @@ -140,7 +138,6 @@ const Cache = t.mock('../../../lib/commands/cache.js', { npmlog, pacote, rimraf, - '../../../lib/utils/usage.js': usageUtil, }) const npm = mockNpm({ @@ -161,7 +158,7 @@ const cache = new Cache(npm) t.test('cache no args', async t => { await t.rejects( cache.exec([]), - 'usage instructions', + { code: 'EUSAGE' }, 'should throw usage instructions' ) }) @@ -194,7 +191,10 @@ t.test('cache add no arg', async t => { await t.rejects( cache.exec(['add']), - { code: 'EUSAGE' }, + { + code: 'EUSAGE', + message: 'Usage: First argument to `add` is required', + }, 'throws usage error' ) t.strictSame(logOutput, [ diff --git a/test/lib/commands/help-search.js b/test/lib/commands/help-search.js index 9faa38a32fd02..406977b622a2f 100644 --- a/test/lib/commands/help-search.js +++ b/test/lib/commands/help-search.js @@ -103,8 +103,7 @@ t.test('npm help-search long output with color', async t => { }) t.test('npm help-search no args', async t => { - await helpSearch.exec([]) - t.match(OUTPUT, /npm help-search/, 'outputs usage') + t.rejects(helpSearch.exec([]), /npm help-search/, 'outputs usage') }) t.test('npm help-search no matches', async t => { diff --git a/test/lib/commands/pkg.js b/test/lib/commands/pkg.js index 784ea747010f4..49234e4cce323 100644 --- a/test/lib/commands/pkg.js +++ b/test/lib/commands/pkg.js @@ -196,7 +196,7 @@ t.test('set no args', async t => { }) await t.rejects( pkg.exec(['set']), - { code: 'EPKGSET' }, + { code: 'EUSAGE' }, 'should throw an error if no args' ) }) @@ -207,7 +207,7 @@ t.test('set missing value', async t => { }) await t.rejects( pkg.exec(['set', 'key=']), - { code: 'EPKGSET' }, + { code: 'EUSAGE' }, 'should throw an error if missing value' ) }) @@ -218,7 +218,7 @@ t.test('set missing key', async t => { }) await t.rejects( pkg.exec(['set', '=value']), - { code: 'EPKGSET' }, + { code: 'EUSAGE' }, 'should throw an error if missing key' ) }) @@ -424,7 +424,7 @@ t.test('delete no args', async t => { }) await t.rejects( pkg.exec(['delete']), - { code: 'EPKGDELETE' }, + { code: 'EUSAGE' }, 'should throw an error if deleting no args' ) }) @@ -435,7 +435,7 @@ t.test('delete invalid key', async t => { }) await t.rejects( pkg.exec(['delete', '']), - { code: 'EPKGDELETE' }, + { code: 'EUSAGE' }, 'should throw an error if deleting invalid args' ) })