Skip to content

Commit

Permalink
fix: make prefixed usage errors more consistent
Browse files Browse the repository at this point in the history
PR-URL: #3987
Credit: @lukekarrys
Close: #3987
Reviewed-by: @wraithgar
  • Loading branch information
lukekarrys committed Nov 4, 2021
1 parent a0d35ff commit 22230ef
Show file tree
Hide file tree
Showing 16 changed files with 38 additions and 54 deletions.
12 changes: 4 additions & 8 deletions lib/base-command.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
})
}
Expand Down
9 changes: 2 additions & 7 deletions lib/commands/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

Expand Down Expand Up @@ -161,14 +161,9 @@ class Cache extends BaseCommand {
// npm cache add <tarball>...
// npm cache add <folder>...
async add (args) {
const usage = 'Usage:\n' +
' npm cache add <tarball-url>...\n' +
' npm cache add <pkg>@<ver>...\n' +
' npm cache add <tarball>...\n' +
' npm cache add <folder>...\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)
Expand Down
16 changes: 6 additions & 10 deletions lib/commands/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]) {
Expand All @@ -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}`]
}
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/exec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions lib/commands/explore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/help-search.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
})
}
Expand Down
10 changes: 2 additions & 8 deletions lib/commands/pkg.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down
2 changes: 1 addition & 1 deletion lib/commands/star.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/team.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class Team extends BaseCommand {
return this.listTeams(entity, opts)
}
default:
throw this.usage
throw this.usageError()
}
})
}
Expand Down
4 changes: 4 additions & 0 deletions packages/libnpmdiff/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
10 changes: 5 additions & 5 deletions test/lib/commands/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ''
Expand Down Expand Up @@ -140,7 +138,6 @@ const Cache = t.mock('../../../lib/commands/cache.js', {
npmlog,
pacote,
rimraf,
'../../../lib/utils/usage.js': usageUtil,
})

const npm = mockNpm({
Expand All @@ -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'
)
})
Expand Down Expand Up @@ -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, [
Expand Down
3 changes: 1 addition & 2 deletions test/lib/commands/help-search.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down
10 changes: 5 additions & 5 deletions test/lib/commands/pkg.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
)
})
Expand All @@ -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'
)
})
Expand All @@ -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'
)
})
Expand Down Expand Up @@ -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'
)
})
Expand All @@ -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'
)
})
Expand Down

0 comments on commit 22230ef

Please sign in to comment.