Skip to content

Parsing fixes #12376

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

Merged
hansl merged 3 commits into from
Sep 27, 2018
Merged

Parsing fixes #12376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
930dae7
fix(@angular/cli): allow empty string arguments
hansl Sep 26, 2018
c3e2ab7
fix(@angular/cli): allow -a=value arguments
hansl Sep 26, 2018
2ffde25
fix(@angular/cli): numerical flags should not give 0 if empty
hansl Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions packages/angular/cli/models/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ function _coerceType(str: string | undefined, type: OptionType, v?: Value): Valu
case OptionType.Number:
if (str === undefined) {
return 0;
} else if (str === '') {
return undefined;
} else if (Number.isFinite(+str)) {
return +str;
} else {
Expand Down Expand Up @@ -269,10 +271,6 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
const errors: string[] = [];

for (let arg = args.shift(); arg !== undefined; arg = args.shift()) {
if (!arg) {
break;
}

if (arg == '--') {
// If we find a --, we're done.
leftovers.push(...args);
Expand All @@ -285,6 +283,12 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
// Argument is of form -abcdef. Starts at 1 because we skip the `-`.
for (let i = 1; i < arg.length; i++) {
const flag = arg[i];
// If the next character is an '=', treat it as a long flag.
if (arg[i + 1] == '=') {
const f = '--' + flag + arg.slice(i + 1);
_assignOption(f, args, options, parsedOptions, positionals, leftovers, ignored, errors);
break;
}
// Treat the last flag as `--a` (as if full flag but just one letter). We do this in
// the loop because it saves us a check to see if the arg is just `-`.
if (i == arg.length - 1) {
Expand All @@ -306,6 +310,8 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
}

// Deal with positionals.
// TODO(hansl): this is by far the most complex piece of code in this file. Try to refactor it
// simpler.
if (positionals.length > 0) {
let pos = 0;
for (let i = 0; i < positionals.length;) {
Expand All @@ -315,10 +321,11 @@ export function parseArguments(args: string[], options: Option[] | null): Argume

// We do this with a found flag because more than 1 option could have the same positional.
for (const option of options) {
// If any option has this positional and no value, we need to remove it.
// If any option has this positional and no value, AND fit the type, we need to remove it.
if (option.positional === pos) {
if (parsedOptions[option.name] === undefined) {
parsedOptions[option.name] = positionals[i];
const coercedValue = _coerce(positionals[i], option, parsedOptions[option.name]);
if (parsedOptions[option.name] === undefined && coercedValue !== undefined) {
parsedOptions[option.name] = coercedValue;
found = true;
} else {
incrementI = false;
Expand Down
20 changes: 17 additions & 3 deletions packages/angular/cli/models/parser_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ describe('parseArguments', () => {
{ name: 'arr', aliases: [ 'a' ], type: OptionType.Array, description: '' },
{ name: 'p1', positional: 0, aliases: [], type: OptionType.String, description: '' },
{ name: 'p2', positional: 1, aliases: [], type: OptionType.String, description: '' },
{ name: 'p3', positional: 2, aliases: [], type: OptionType.Number, description: '' },
{ name: 't1', aliases: [], type: OptionType.Boolean,
types: [OptionType.Boolean, OptionType.String], description: '' },
{ name: 't2', aliases: [], type: OptionType.Boolean,
Expand All @@ -35,6 +36,7 @@ describe('parseArguments', () => {
const tests: { [test: string]: Partial<Arguments> | ['!!!', Partial<Arguments>, string[]] } = {
'--bool': { bool: true },
'--bool=1': ['!!!', {}, ['--bool=1']],
'--bool ': { bool: true, p1: '' },
'-- --bool=1': { '--': ['--bool=1'] },
'--bool=yellow': ['!!!', {}, ['--bool=yellow']],
'--bool=true': { bool: true },
Expand Down Expand Up @@ -62,20 +64,31 @@ describe('parseArguments', () => {
'val1 --num=1 val2': { num: 1, p1: 'val1', p2: 'val2' },
'--p1=val1 --num=1 val2': { num: 1, p1: 'val1', p2: 'val2' },
'--p1=val1 --num=1 --p2=val2 val3': { num: 1, p1: 'val1', p2: 'val2', '--': ['val3'] },
'--bool val1 --etc --num val2 --v': { bool: true, num: 0, p1: 'val1', p2: 'val2',
'--': ['--etc', '--v'] },
'--bool val1 --etc --num val2 --v': [
'!!!',
{ bool: true, p1: 'val1', p2: 'val2', '--': ['--etc', '--v'] },
['--num' ],
],
'--bool val1 --etc --num=1 val2 --v': { bool: true, num: 1, p1: 'val1', p2: 'val2',
'--': ['--etc', '--v'] },
'--arr=a --arr=b --arr c d': { arr: ['a', 'b', 'c'], p1: 'd' },
'--arr=1 --arr --arr c d': { arr: ['1', '', 'c'], p1: 'd' },
'--arr=1 --arr --arr c d e': { arr: ['1', '', 'c'], p1: 'd', p2: 'e' },
'--str=1': { str: '1' },
'--str=': { str: '' },
'--str ': { str: '' },
'--str ': { str: '', p1: '' },
'--str ': { str: '', p1: '', p2: '', '--': [''] },
'--hello-world=1': { helloWorld: '1' },
'--hello-bool': { helloBool: true },
'--helloBool': { helloBool: true },
'--no-helloBool': { helloBool: false },
'--noHelloBool': { helloBool: false },
'--noBool': { bool: false },
'-b': { bool: true },
'-b=true': { bool: true },
'-sb': { bool: true, str: '' },
'-s=b': { str: 'b' },
'-bs': { bool: true, str: '' },
'--t1=true': { t1: true },
'--t1': { t1: true },
Expand Down Expand Up @@ -114,12 +127,13 @@ describe('parseArguments', () => {
'--e3': { e3: true },
'--e3 true': { e3: true },
'--e3=true': { e3: true },
'a b c 1': { p1: 'a', p2: 'b', '--': ['c', '1'] },
};

Object.entries(tests).forEach(([str, expected]) => {
it(`works for ${str}`, () => {
try {
const actual = parseArguments(str.split(/\s+/), options);
const actual = parseArguments(str.split(' '), options);

expect(Array.isArray(expected)).toBe(false);
expect(actual).toEqual(expected as Arguments);
Expand Down