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

fix(positional): positional strings no longer drop decimals #1761

Merged
merged 4 commits into from Sep 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion lib/command.ts
Expand Up @@ -221,7 +221,7 @@ export function command (
// to simplify the parsing of positionals in commands,
// we temporarily populate '--' rather than _, with arguments
const populateDoubleDash = !!yargs.getOptions().configuration['populate--']
if (!populateDoubleDash) yargs._copyDoubleDash(innerArgv)
yargs._postProcess(innerArgv, populateDoubleDash)

innerArgv = applyMiddleware(innerArgv, yargs, middlewares, false)
let handlerResult
Expand Down
2 changes: 1 addition & 1 deletion lib/platform-shims/deno.ts
Expand Up @@ -6,7 +6,7 @@ import { basename, dirname, extname, posix } from 'https://deno.land/std/path/mo

import cliui from 'https://deno.land/x/cliui@v7.0.0-deno/deno.ts'
import escalade from 'https://deno.land/x/escalade@v3.0.3/sync.ts'
import Parser from 'https://deno.land/x/yargs_parser@v19.0.1-deno/deno.ts'
import Parser from 'https://deno.land/x/yargs_parser@v20.2.0-deno/deno.ts'
import y18n from 'https://deno.land/x/y18n@v5.0.0-deno/deno.ts'
import { YError } from '../../build/lib/yerror.js'

Expand Down
3 changes: 3 additions & 0 deletions lib/typings/yargs-parser-types.ts
Expand Up @@ -51,6 +51,8 @@ export interface Configuration {
'nargs-eats-options': boolean;
/** The prefix to use for negated boolean variables. Default is `'no-'` */
'negation-prefix': string;
/** Should positional values that look like numbers be parsed? Default is `true` */
'parse-positional-numbers': boolean;
/** Should keys that look like numbers be treated as such? Default is `true` */
'parse-numbers': boolean;
/** Should unparsed flags be stored in -- or _? Default is `false` */
Expand Down Expand Up @@ -132,6 +134,7 @@ export interface Parser {
detailed(args: ArgsInput, opts?: Partial<Options>): DetailedArguments;
camelCase(str: string): string;
decamelize(str: string, joinString?: string): string;
looksLikeNumber(x: null | undefined | number | string): boolean;
}
export declare type StringFlag = Dictionary<string[]>;
export declare type BooleanFlag = Dictionary<boolean>;
Expand Down
68 changes: 54 additions & 14 deletions lib/yargs-factory.ts
Expand Up @@ -6,7 +6,7 @@
import { CommandInstance, CommandHandler, CommandBuilderDefinition, CommandBuilder, CommandHandlerCallback, FinishCommandHandler, command as Command, CommandHandlerDefinition } from './command.js'
import { Dictionary, assertNotStrictEqual, KeyOf, DictionaryKeyof, ValueOf, objectKeys, assertSingleKey, RequireDirectoryOptions, PlatformShim, RequireType } from './typings/common-types.js'
import {
Arguments as ParserArguments,
ArgsOutput,
DetailedArguments as ParserDetailedArguments,
Configuration as ParserConfiguration,
Options as ParserOptions,
Expand Down Expand Up @@ -1218,7 +1218,7 @@ function Yargs (processArgs: string | string[] = [], cwd = shim.process.cwd(), p
'populate--': true
})
const parsed = shim.Parser.detailed(args, Object.assign({}, options, {
configuration: config
configuration: { 'parse-positional-numbers': false, ...config }
})) as DetailedArguments

let argv = parsed.argv as Arguments
Expand All @@ -1235,7 +1235,7 @@ function Yargs (processArgs: string | string[] = [], cwd = shim.process.cwd(), p
// are two passes through the parser. If completion
// is being performed short-circuit on the first pass.
if (shortCircuit) {
return (populateDoubleDash || _calledFromCommand) ? argv : self._copyDoubleDash(argv)
return self._postProcess(argv, populateDoubleDash, _calledFromCommand)
}

// if there's a handler associated with a
Expand Down Expand Up @@ -1269,7 +1269,7 @@ function Yargs (processArgs: string | string[] = [], cwd = shim.process.cwd(), p
// the deepest command first; we keep track of the position in the
// argv._ array that is currently being executed.
const innerArgv = command.runCommand(cmd, self, parsed, i + 1)
return populateDoubleDash ? innerArgv : self._copyDoubleDash(innerArgv)
return self._postProcess(innerArgv, populateDoubleDash)
} else if (!firstUnknownCommand && cmd !== completionCommand) {
firstUnknownCommand = cmd
break
Expand All @@ -1279,7 +1279,7 @@ function Yargs (processArgs: string | string[] = [], cwd = shim.process.cwd(), p
// run the default command, if defined
if (command.hasDefaultCommand() && !skipDefaultCommand) {
const innerArgv = command.runCommand(null, self, parsed)
return populateDoubleDash ? innerArgv : self._copyDoubleDash(innerArgv)
return self._postProcess(innerArgv, populateDoubleDash)
}

// recommend a command if recommendCommands() has
Expand All @@ -1297,7 +1297,7 @@ function Yargs (processArgs: string | string[] = [], cwd = shim.process.cwd(), p
}
} else if (command.hasDefaultCommand() && !skipDefaultCommand) {
const innerArgv = command.runCommand(null, self, parsed)
return populateDoubleDash ? innerArgv : self._copyDoubleDash(innerArgv)
return self._postProcess(innerArgv, populateDoubleDash)
}

// we must run completions first, a user might
Expand All @@ -1316,7 +1316,7 @@ function Yargs (processArgs: string | string[] = [], cwd = shim.process.cwd(), p

self.exit(0)
})
return (populateDoubleDash || _calledFromCommand) ? argv : self._copyDoubleDash(argv)
return self._postProcess(argv, !populateDoubleDash, _calledFromCommand)
}

// Handle 'help' and 'version' options
Expand Down Expand Up @@ -1360,25 +1360,54 @@ function Yargs (processArgs: string | string[] = [], cwd = shim.process.cwd(), p
else throw err
}

return (populateDoubleDash || _calledFromCommand) ? argv : self._copyDoubleDash(argv)
return self._postProcess(argv, populateDoubleDash, _calledFromCommand)
}

// Applies a couple post processing steps that are easier to perform
// as a final step, rather than
self._postProcess = function (argv: Arguments | Promise<Arguments>, populateDoubleDash: boolean, calledFromCommand = false): any {
if (isPromise(argv)) return argv
if (calledFromCommand) return argv
if (!populateDoubleDash) {
argv = self._copyDoubleDash(argv)
}
const parsePositionalNumbers = self.getParserConfiguration()['parse-positional-numbers'] || self.getParserConfiguration()['parse-positional-numbers'] === undefined
if (parsePositionalNumbers) {
argv = self._parsePositionalNumbers(argv)
}
return argv
}

// to simplify the parsing of positionals in commands,
// we temporarily populate '--' rather than _, with arguments
// after the '--' directive. After the parse, we copy these back.
self._copyDoubleDash = function (argv: Arguments | Promise<Arguments>): any {
if (isPromise(argv) || !argv._ || !argv['--']) return argv
self._copyDoubleDash = function (argv: Arguments): any {
if (!argv._ || !argv['--']) return argv
argv._.push.apply(argv._, argv['--'])

// TODO(bcoe): refactor command parsing such that this delete is not
// necessary: https://github.com/yargs/yargs/issues/1482
// We catch an error here, in case someone has called Object.seal()
// on the parsed object, see: https://github.com/babel/babel/pull/10733
try {
delete argv['--']
} catch (_err) {}

return argv
}

// We wait to coerce numbers for positionals until after the initial parse.
// This allows commands to configure number parsing on a positional by
// positional basis:
self._parsePositionalNumbers = function (argv: Arguments): any {
const args: (string | number)[] = argv['--'] ? argv['--'] : argv._

for (let i = 0, arg; (arg = args[i]) !== undefined; i++) {
if (shim.Parser.looksLikeNumber(arg) && Number.isSafeInteger(Math.floor(parseFloat(`${arg}`)))) {
args[i] = Number(arg)
}
}
return argv
}

self._runValidation = function runValidation (argv, aliases, positionalMap, parseErrors, isDefaultCommand = false) {
if (parseErrors) throw new YError(parseErrors.message)
validation.nonOptionCount(argv)
Expand Down Expand Up @@ -1425,7 +1454,12 @@ export interface YargsInstance {
argv: Arguments
customScriptName: boolean
parsed: DetailedArguments | false
_copyDoubleDash<T extends Arguments | Promise<Arguments>> (argv: T): T
// The methods below are called after the parse in yargs-parser is complete
// and perform cleanup on the object generated:
_postProcess<T extends Arguments | Promise<Arguments>> (argv: T, populateDoubleDash: boolean, calledFromCommand?: boolean): T
_copyDoubleDash<T extends Arguments> (argv: T): T
_parsePositionalNumbers<T extends Arguments> (argv: T): T

_getLoggerInstance (): LoggerInstance
_getParseContext(): Object
_hasOutput (): boolean
Expand Down Expand Up @@ -1740,9 +1774,15 @@ interface ParseCallback {
(err: YError | string | undefined | null, argv: Arguments | Promise<Arguments>, output: string): void
}

export interface Arguments extends ParserArguments {
export interface Arguments {
/** The script name or node command */
$0: string
/** Non-option arguments */
_: ArgsOutput;
/** Arguments after the end-of-options flag `--` */
'--'?: ArgsOutput;
/** All remaining options */
[argName: string]: any;
}

export interface DetailedArguments extends ParserDetailedArguments {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -42,7 +42,7 @@
"require-directory": "^2.1.1",
"string-width": "^4.2.0",
"y18n": "^5.0.1",
"yargs-parser": "^20.0.0"
"yargs-parser": "^20.2.0"
},
"devDependencies": {
"@types/chai": "^4.2.11",
Expand Down
15 changes: 14 additions & 1 deletion test/deno/yargs.test.ts
@@ -1,10 +1,11 @@
/* global Deno */

import {
assertEquals,
assertMatch
} from 'https://deno.land/std/testing/asserts.ts'
import yargs from '../../deno.ts'
import { Arguments } from '../../types.ts'
import { Arguments, YargsType } from '../../types.ts'

Deno.test('demandCommand(1) throw error if no command provided', () => {
let err: Error|null = null
Expand All @@ -25,3 +26,15 @@ Deno.test('guesses version # based on package.json', () => {
})
assertMatch('' + output, /[0-9]+\.[0-9]+\.[0-9]+/)
})

// Handling of strings that look like numbers, see:
// https://github.com/yargs/yargs/issues/1758
Deno.test('does not drop .0 if positional is configured as string', async () => {
const argv = await yargs(['cmd', '33.0'])
.command('cmd [str]', 'a command', (yargs: YargsType) => {
return yargs.positional('str', {
type: 'string'
})
}).parse() as Arguments
assertEquals(argv.str, '33.0')
})
13 changes: 13 additions & 0 deletions test/esm/yargs-test.mjs
Expand Up @@ -21,4 +21,17 @@ describe('ESM', () => {
assert.match(err.message, /not supported yet for ESM/)
})
})
// Handling of strings that look like numbers, see:
// https://github.com/yargs/yargs/issues/1758
describe('bug #1758', () => {
it('does not drop .0 if positional is configured as string', () => {
const argv = yargs('cmd 33.0')
.command('cmd [str]', 'a command', (yargs) => {
return yargs.positional('str', {
type: 'string'
})
}).parse()
assert.strictEqual(argv.str, '33.0')
})
})
})
34 changes: 32 additions & 2 deletions test/yargs.cjs
Expand Up @@ -1803,14 +1803,14 @@ describe('yargs dsl tests', () => {
const argv = yargs('--foo.bar 1 --no-baz 2')
.parserConfiguration({ 'unknown-options-as-args': true })
.parse()
argv._.should.deep.eql(['--foo.bar', '1', '--no-baz', '2'])
argv._.should.deep.eql(['--foo.bar', 1, '--no-baz', 2])
const argv2 = yargs('foo --foo.bar --cool 1 --no-baz 2')
.command('foo', 'my foo command', (yargs) => {
yargs.boolean('cool')
}, () => {})
.parserConfiguration({ 'unknown-options-as-args': true })
.parse()
argv2._.should.deep.eql(['foo', '--foo.bar', '1', '--no-baz', '2'])
argv2._.should.deep.eql(['foo', '--foo.bar', 1, '--no-baz', 2])
argv2.cool.should.equal(true)
})
})
Expand Down Expand Up @@ -2595,4 +2595,34 @@ describe('yargs dsl tests', () => {
}).to.throw(/yargs supports a minimum Node.js version of 55/)
delete process.env.YARGS_MIN_NODE_VERSION
})

// Handling of strings that look like numbers, see:
// https://github.com/yargs/yargs/issues/1758
describe('bug #1758', () => {
it('does not drop .0 if flag is configured as string', () => {
const argv = yargs('cmd --foo 33.0')
.command('cmd [str]', 'a command', (yargs) => {
return yargs.option('foo', {
type: 'string'
})
}).parse()
argv.foo.should.equal('33.0')
})

it('does not drop .0 if positional is configured as string', () => {
const argv = yargs('cmd 33.0')
.command('cmd [str]', 'a command', (yargs) => {
return yargs.positional('str', {
type: 'string'
})
}).parse()
argv.str.should.equal('33.0')
})

it('continues to parse values in _ as numbers, when they look like numbers', () => {
const argv = yargs('hello 33.0').parse()
argv._[0].should.equal('hello')
argv._[1].should.equal(33)
})
})
})