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.

Sign up for GitHub

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

Jump to bottom

feat: requiresArg is now simply an alias for nargs(1) #1054

Merged
merged 1 commit into from
Jan 22, 2018
Merged

feat: requiresArg is now simply an alias for nargs(1) #1054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 0 additions & 34 deletions lib/validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,40 +54,6 @@ module.exports = function validation (yargs, usage, y18n) {
}
}

// make sure that any args that require an
// value (--foo=bar), have a value.
self.missingArgumentValue = function missingArgumentValue (argv) {
const options = yargs.getOptions()
if (options.requiresArg.length > 0) {
const missingRequiredArgs = []
const defaultValues = new Set([true, false, ''])

options.requiresArg.forEach((key) => {
// if the argument is not set in argv no need to check
// whether a right-hand-side has been provided.
if (!argv.hasOwnProperty(key)) return

const value = argv[key]
// if a value is explicitly requested,
// flag argument as missing if it does not
// look like foo=bar was entered.
if (defaultValues.has(value) ||
(Array.isArray(value) && !value.length)) {
missingRequiredArgs.push(key)
}
})

if (missingRequiredArgs.length > 0) {
usage.fail(__n(
'Missing argument value: %s',
'Missing argument values: %s',
missingRequiredArgs.length,
missingRequiredArgs.join(', ')
))
}
}
}

// make sure all the required arguments are present.
self.requiredArguments = function requiredArguments (argv) {
const demandedOptions = yargs.getDemandedOptions()
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"string-width": "^2.0.0",
"which-module": "^2.0.0",
"y18n": "^3.2.1",
"yargs-parser": "^8.1.0"
"yargs-parser": "^9.0.2"
},
"devDependencies": {
"chai": "^4.1.2",
Expand Down
8 changes: 4 additions & 4 deletions test/usage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ describe('usage tests', () => {
' --version Show version number [boolean]',
' --foo, -f foo option',
' --bar, -b bar option',
'Missing argument value: foo'
'Not enough arguments following: f'
])
})

Expand Down Expand Up @@ -722,7 +722,7 @@ describe('usage tests', () => {
' --version Show version number [boolean]',
' --foo, -f foo option',
' --bar, -b bar option',
'Missing argument values: foo, bar'
'Not enough arguments following: bar'
])
})
})
Expand Down Expand Up @@ -754,7 +754,7 @@ describe('usage tests', () => {
' --version Show version number [boolean]',
' --foo, -f foo option',
' --bar, -b bar option',
'Missing argument value: foo'
'Not enough arguments following: f'
])
})
})
Expand All @@ -769,7 +769,7 @@ describe('usage tests', () => {
.argv
)

r.errors[1].should.equal('Missing argument values: foo, bar')
r.errors[1].should.equal('Not enough arguments following: bar')
})
})

Expand Down
6 changes: 3 additions & 3 deletions test/validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ describe('validation tests', () => {
.option('w', {type: 'string', requiresArg: true})
.parse('-w', (err, argv, output) => {
expect(err).to.exist
expect(err).to.have.property('message', 'Missing argument value: w')
expect(err).to.have.property('message', 'Not enough arguments following: w')
return done()
})
})
Expand All @@ -451,7 +451,7 @@ describe('validation tests', () => {
.option('w', {type: 'boolean', requiresArg: true})
.parse('-w', (err, argv, output) => {
expect(err).to.exist
expect(err).to.have.property('message', 'Missing argument value: w')
expect(err).to.have.property('message', 'Not enough arguments following: w')
return done()
})
})
Expand All @@ -461,7 +461,7 @@ describe('validation tests', () => {
.option('w', {type: 'array', requiresArg: true})
.parse('-w', (err, argv, output) => {
expect(err).to.exist
expect(err).to.have.property('message', 'Missing argument value: w')
expect(err).to.have.property('message', 'Not enough arguments following: w')
return done()
})
})
Expand Down
1 change: 0 additions & 1 deletion test/yargs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@ describe('yargs dsl tests', () => {
defaultDescription: {},
choices: {},
coerce: {},
requiresArg: [],
skipValidation: [],
count: [],
normalize: [],
Expand Down
16 changes: 2 additions & 14 deletions yargs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ function Yargs (processArgs, cwd, parentRequire) {
groups = {}

const arrayOptions = [
'array', 'boolean', 'string', 'requiresArg', 'skipValidation',
'array', 'boolean', 'string', 'skipValidation',
'count', 'normalize', 'number'
]

Expand Down Expand Up @@ -204,7 +204,7 @@ function Yargs (processArgs, cwd, parentRequire) {

self.requiresArg = function (keys) {
argsert('<array|string>', [keys], arguments.length)
populateParserHintArray('requiresArg', keys)
populateParserHintObject(self.nargs, false, 'narg', keys, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is calling self.nargs(keys, 1) too cute, here?

Both ways seem fine, the tradeoff being a duplicated call to populateParserHintObject vs a redundant argsert in the nested call...

Copy link
Member Author

Choose a reason for hiding this comment

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

@evocateur it doesn't seem too cute; I like your idea of calling self.nargs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was envisioning it being the entire contents of the method, not in addition to the duplicate argsert+populate call.

return self
}

Expand Down Expand Up @@ -965,17 +965,6 @@ function Yargs (processArgs, cwd, parentRequire) {
options.__ = y18n.__
options.configuration = pkgUp()['yargs'] || {}

// numbers are defaulted to `undefined`, which makes it impossible to verify requiresArg
// so _unlike_ other types, we will implicitly add them to narg when requiresArg is true
const numberTyped = new Set(options.number)
options.requiresArg
.filter(key => numberTyped.has(key))
.forEach(key => {
// if narg _and_ requiresArg are configured for an option,
// we should probably throw an error somewhere indicating conflicting configuration
options.narg[key] = 1
})

const parsed = Parser.detailed(args, options)
let argv = parsed.argv
if (parseContext) argv = Object.assign({}, argv, parseContext)
Expand Down Expand Up @@ -1118,7 +1107,6 @@ function Yargs (processArgs, cwd, parentRequire) {
self._runValidation = function runValidation (argv, aliases, positionalMap, parseErrors) {
if (parseErrors) throw new YError(parseErrors.message)
validation.nonOptionCount(argv)
validation.missingArgumentValue(argv)
validation.requiredArguments(argv)
if (strict) validation.unknownArguments(argv, aliases, positionalMap)
validation.customChecks(argv, aliases)
Expand Down