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

feat: introduces strictCommands() subset of strict mode #1540

Merged
merged 5 commits into from Feb 18, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/api.md
Expand Up @@ -1401,6 +1401,16 @@ corresponding description, will be reported as an error.

Unrecognized commands will also be reported as errors.

.strictCommands([enabled=true])
---------

Similar to `.strict()`, except that it only applies to unrecognized commands. A
user can still provide arbitrary options, but unknown positional commands
will raise an error.

_Note: if you use `.demandCommand()` or `.demandCommand(1)`, in conjunction
with defining commands, `.strictCommands()` is enabled automatically.

<a name="string"></a>.string(key)
------------

Expand Down
33 changes: 33 additions & 0 deletions lib/validation.js
Expand Up @@ -16,6 +16,13 @@ module.exports = function validation (yargs, usage, y18n) {
const demandedCommands = yargs.getDemandedCommands()
// don't count currently executing commands
const _s = argv._.length - yargs.getContext().commands.length
const commandKeys = yargs.getCommandInstance().getCommands()

// for the special case of yargs.demandCommand() and yargs.demandCommand(1), if
// yargs has been configured with commands, we automatically enable strictCommands.
if (commandKeys.length && demandedCommands._ && demandedCommands._.min === 1 && demandedCommands._.max === Infinity) {
yargs.strictCommands()
evocateur marked this conversation as resolved.
Show resolved Hide resolved
}

if (demandedCommands._ && (_s < demandedCommands._.min || _s > demandedCommands._.max)) {
if (_s < demandedCommands._.min) {
Expand Down Expand Up @@ -120,6 +127,32 @@ module.exports = function validation (yargs, usage, y18n) {
}
}

self.unknownCommands = function unknownCommands (argv, aliases, positionalMap) {
const commandKeys = yargs.getCommandInstance().getCommands()
const unknown = []
const currentContext = yargs.getContext()

if ((currentContext.commands.length > 0) || (commandKeys.length > 0)) {
argv._.slice(currentContext.commands.length).forEach((key) => {
if (commandKeys.indexOf(key) === -1) {
unknown.push(key)
}
})
}

if (unknown.length > 0) {
usage.fail(__n(
'Unknown argument: %s',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be "Unknown command"? The error as written looks too similar to the "unknown argument" output.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are 100% correct ... the reason I was thinking of using Unknown argument: was to avoid having to update all the languages yargs has been translated into, alternatively we could introduce the new phrase and let folks gradually submit translations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's more important to be precise with error messages, even if it adds another untranslated string.

Copy link
Member

Choose a reason for hiding this comment

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

For fr.json, this would be:

  "Unknown command: %s": {
    "one": "Commande inconnue : %s",
    "other": "Commandes inconnues : %s"
  },

The space before ":" is intentional, as we do have a space in French before every punctuation sign drawn in more than one piece (":", "?", "!", ";", etc. but not ","). I see I will have some cleanup to do in fr.json

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 @mleguen mind making a PR for the french translation in a separate PR, as I know we've been fixing a few issues with translations recently.

Copy link
Member

Choose a reason for hiding this comment

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

@bcoe No problem. I will do it when I will rebase the PR for french translations, once this one is merged.

'Unknown arguments: %s',
unknown.length,
unknown.join(', ')
))
return true
} else {
return false
}
}

// check for a key that is not an alias, or for which every alias is new,
// implying that it was invented by the parser, e.g., during camelization
self.isValidAndSomeAliasIsNotNew = function isValidAndSomeAliasIsNotNew (key, aliases) {
Expand Down
80 changes: 80 additions & 0 deletions test/validation.js
Expand Up @@ -939,4 +939,84 @@ describe('validation tests', () => {
.parse()
})
})

describe('strictCommands', () => {
bcoe marked this conversation as resolved.
Show resolved Hide resolved
it('succeeds in parse if command is known', () => {
const parsed = yargs('foo -a 10')
.strictCommands()
.command('foo', 'foo command')
.parse()
parsed.a.should.equal(10)
parsed._.should.eql(['foo'])
})

it('succeeds in parse if top level and inner command are known', () => {
const parsed = yargs('foo bar --cool beans')
.strictCommands()
.command('foo', 'foo command', (yargs) => {
yargs.command('bar')
})
.parse()
parsed.cool.should.equal('beans')
parsed._.should.eql(['foo', 'bar'])
})

it('fails with error if command is unknown', (done) => {
yargs('blerg -a 10')
.strictCommands()
.command('foo', 'foo command')
.fail((msg) => {
msg.should.equal('Unknown argument: blerg')
return done()
})
.parse()
})

it('fails with error if inner command is unknown', (done) => {
yargs('foo blarg --cool beans')
.strictCommands()
.command('foo', 'foo command', (yargs) => {
yargs.command('bar')
})
.fail((msg) => {
msg.should.equal('Unknown argument: blarg')
return done()
})
.parse()
})

it('enables strict commands if commands used in conjunction with demandCommand', (done) => {
yargs('blerg -a 10')
.demandCommand()
.command('foo', 'foo command')
.fail((msg) => {
msg.should.equal('Unknown argument: blerg')
return done()
})
.parse()
})

it('does not apply implicit strictCommands to inner commands', () => {
const parse = yargs('foo blarg --cool beans')
.demandCommand()
.command('foo', 'foo command', (yargs) => {
yargs.command('bar')
})
.parse()
parse.cool.should.equal('beans')
parse._.should.eql(['foo', 'blarg'])
})

it('allows strictCommands to be applied to inner commands', (done) => {
yargs('foo blarg')
.command('foo', 'foo command', (yargs) => {
yargs.command('bar').strictCommands()
})
.fail((msg) => {
msg.should.equal('Unknown argument: blarg')
return done()
})
.parse()
})
})
})
18 changes: 17 additions & 1 deletion yargs.js
Expand Up @@ -167,6 +167,7 @@ function Yargs (processArgs, cwd, parentRequire) {
validation.freeze()
command.freeze()
frozen.strict = strict
frozen.strictCommands = strictCommands
frozen.completionCommand = completionCommand
frozen.output = output
frozen.exitError = exitError
Expand All @@ -190,6 +191,7 @@ function Yargs (processArgs, cwd, parentRequire) {
validation.unfreeze()
command.unfreeze()
strict = frozen.strict
strictCommands = frozen.strictCommands
completionCommand = frozen.completionCommand
parseFn = frozen.parseFn
parseContext = frozen.parseContext
Expand Down Expand Up @@ -795,6 +797,14 @@ function Yargs (processArgs, cwd, parentRequire) {
}
self.getStrict = () => strict

let strictCommands = false
self.strictCommands = function (enabled) {
argsert('[boolean]', [enabled], arguments.length)
strictCommands = enabled !== false
return self
}
self.getStrictCommands = () => strictCommands

let parserConfig = {}
self.parserConfiguration = function parserConfiguration (config) {
argsert('<object>', [config], arguments.length)
Expand Down Expand Up @@ -1219,7 +1229,13 @@ function Yargs (processArgs, cwd, parentRequire) {
if (parseErrors) throw new YError(parseErrors.message)
validation.nonOptionCount(argv)
validation.requiredArguments(argv)
if (strict) validation.unknownArguments(argv, aliases, positionalMap)
let failedStrictCommands = false
if (strictCommands) {
failedStrictCommands = validation.unknownCommands(argv, aliases, positionalMap)
}
if (strict && !failedStrictCommands) {
validation.unknownArguments(argv, aliases, positionalMap)
}
validation.customChecks(argv, aliases)
validation.limitedChoices(argv)
validation.implications(argv)
Expand Down