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

Make positionals opt-in when strict:true #116

Merged
merged 4 commits into from
Apr 18, 2022
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ added: REPLACEME
are encountered, or when arguments are passed that do not match the
`type` configured in `options`.
**Default:** `true`.
* `allowPositionals`: {boolean} Whether this command accepts positional arguments.
**Default:** `false` if `strict` is `true`, otherwise `true`.

* Returns: {Object} An {Object} representing the parsed command line
arguments:
Expand All @@ -38,7 +40,7 @@ added: REPLACEME
* `positionals` {string\[]}, containing positional arguments.

Provides a higher level API for command-line argument parsing than interacting
with `process.argv` directly.
with `process.argv` directly. Takes a specification for the expected arguments and returns a structured object with the parsed options and positionals.

```mjs
import { parseArgs } from 'util';
Expand Down Expand Up @@ -133,7 +135,7 @@ This package was implemented using [tape](https://www.npmjs.com/package/tape) as
## 💡 `process.mainArgs` Proposal

> Note: This can be moved forward independently of the `util.parseArgs()` proposal/work.

### Implementation:

```javascript
Expand All @@ -153,6 +155,7 @@ process.mainArgs = process.argv.slice(process._exec ? 1 : 2)
* `multiple` {boolean} (Optional) If true, when appearing one or more times in `args`, results are collected in an `Array`
* `short` {string} (Optional) A single character alias for an option; When appearing one or more times in `args`; Respects the `multiple` configuration
* `strict` {Boolean} (Optional) A `Boolean` for whether or not to throw an error when unknown options are encountered, `type:'string'` options are missing an options-argument, or `type:'boolean'` options are passed an options-argument; defaults to `true`
* `allowPositionals` {Boolean} (Optional) Whether this command accepts positional arguments. Defaults `false` if `strict` is `true`, otherwise defaults to `true`.
* Returns: {Object} An object having properties:
* `values` {Object}, key:value for each option found. Value is a string for string options, or `true` for boolean options, or an array (of strings or booleans) for options configured as `multiple:true`.
* `positionals` {string[]}, containing [Positionals][]
Expand Down Expand Up @@ -203,7 +206,7 @@ const options = {
},
};
const args = ['-f', 'b'];
const { values, positionals } = parseArgs({ args, options });
const { values, positionals } = parseArgs({ args, options, allowPositionals: true });
// values = { foo: true }
// positionals = ['b']
```
Expand All @@ -213,7 +216,7 @@ const { parseArgs } = require('@pkgjs/parseargs');
// unconfigured
const options = {};
const args = ['-f', '--foo=a', '--bar', 'b'];
const { values, positionals } = parseArgs({ strict: false, args, options });
const { values, positionals } = parseArgs({ strict: false, args, options, allowPositionals: true });
// values = { f: true, foo: 'a', bar: true }
// positionals = ['b']
```
Expand Down Expand Up @@ -273,7 +276,7 @@ const { values, positionals } = parseArgs({ strict: false, args, options });
- no, `-bar` is a short option or options, with expansion logic that follows the
[Utility Syntax Guidelines in POSIX.1-2017](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html). `-bar` expands to `-b`, `-a`, `-r`.
- Is `---foo` the same as `--foo`?
- no
- no
- the first is a long option named `'-foo'`
- the second is a long option named `'foo'`
- Is `-` a positional? ie, `bash some-test.sh | tap -`
Expand Down
13 changes: 11 additions & 2 deletions errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,26 @@ class ERR_PARSE_ARGS_INVALID_OPTION_VALUE extends Error {
}

class ERR_PARSE_ARGS_UNKNOWN_OPTION extends Error {
constructor(option) {
super(`Unknown option '${option}'`);
constructor(option, allowPositionals) {
const suggestDashDash = allowPositionals ? `. To specify a positional argument starting with a '-', place it at the end of the command after '--', as in '-- ${JSON.stringify(option)}` : '';
super(`Unknown option '${option}'${suggestDashDash}`);
this.code = 'ERR_PARSE_ARGS_UNKNOWN_OPTION';
}
}

class ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL extends Error {
constructor(positional) {
super(`Unexpected argument '${positional}'. This command does not take positional arguments`);
this.code = 'ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL';
}
}

module.exports = {
codes: {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_PARSE_ARGS_INVALID_OPTION_VALUE,
ERR_PARSE_ARGS_UNKNOWN_OPTION,
ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL,
}
};
25 changes: 19 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const {
ERR_INVALID_ARG_VALUE,
ERR_PARSE_ARGS_INVALID_OPTION_VALUE,
ERR_PARSE_ARGS_UNKNOWN_OPTION,
ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL,
},
} = require('./errors');

Expand Down Expand Up @@ -86,12 +87,12 @@ function getMainArgs() {
* @param {boolean} strict - show errors, from parseArgs({ strict })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: missing JSDoc for new parameter.

*/
function checkOptionUsage(longOption, optionValue, options,
shortOrLong, strict) {
shortOrLong, strict, allowPositionals) {
// Strict and options are used from local context.
if (!strict) return;

if (!ObjectHasOwn(options, longOption)) {
throw new ERR_PARSE_ARGS_UNKNOWN_OPTION(shortOrLong);
throw new ERR_PARSE_ARGS_UNKNOWN_OPTION(shortOrLong, allowPositionals);
}

const short = optionsGetOwn(options, longOption, 'short');
Expand Down Expand Up @@ -141,11 +142,13 @@ function storeOption(longOption, optionValue, options, values) {
const parseArgs = (config = { __proto__: null }) => {
const args = objectGetOwn(config, 'args') ?? getMainArgs();
const strict = objectGetOwn(config, 'strict') ?? true;
const allowPositionals = objectGetOwn(config, 'allowPositionals') ?? !strict;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A brief comment on change in previous design/expectations.

This means parseArgs may now throw in strict:false mode for userArgs for first time. But it is opt-in and I think make sense when write it first time, and makes sense to read later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I thought this was better than making it so you could have an explicit allowPositionals: false but still get positionals. The alternative is to reject allowPositionals: false in combination with strict: false, but I didn't see much value in that.

const options = objectGetOwn(config, 'options') ?? { __proto__: null };

// Validate input configuration.
validateArray(args, 'args');
validateBoolean(strict, 'strict');
validateBoolean(allowPositionals, 'allowPositionals');
validateObject(options, 'options');
ArrayPrototypeForEach(
ObjectEntries(options),
Expand Down Expand Up @@ -186,6 +189,10 @@ const parseArgs = (config = { __proto__: null }) => {
// Check if `arg` is an options terminator.
// Guideline 10 in https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
if (arg === '--') {
if (!allowPositionals && remainingArgs.length > 0) {
throw new ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL(nextArg);
}

// Everything after a bare '--' is considered a positional argument.
result.positionals = ArrayPrototypeConcat(
result.positionals,
Expand All @@ -204,7 +211,8 @@ const parseArgs = (config = { __proto__: null }) => {
// e.g. '-f', 'bar'
optionValue = ArrayPrototypeShift(remainingArgs);
}
checkOptionUsage(longOption, optionValue, options, arg, strict);
checkOptionUsage(longOption, optionValue, options,
arg, strict, allowPositionals);
storeOption(longOption, optionValue, options, result.values);
continue;
}
Expand Down Expand Up @@ -235,7 +243,7 @@ const parseArgs = (config = { __proto__: null }) => {
const shortOption = StringPrototypeCharAt(arg, 1);
const longOption = findLongOptionForShort(shortOption, options);
const optionValue = StringPrototypeSlice(arg, 2);
checkOptionUsage(longOption, optionValue, options, `-${shortOption}`, strict);
checkOptionUsage(longOption, optionValue, options, `-${shortOption}`, strict, allowPositionals);
storeOption(longOption, optionValue, options, result.values);
continue;
}
Expand All @@ -249,7 +257,8 @@ const parseArgs = (config = { __proto__: null }) => {
// e.g. '--foo', 'bar'
optionValue = ArrayPrototypeShift(remainingArgs);
}
checkOptionUsage(longOption, optionValue, options, arg, strict);
checkOptionUsage(longOption, optionValue, options,
arg, strict, allowPositionals);
storeOption(longOption, optionValue, options, result.values);
continue;
}
Expand All @@ -259,12 +268,16 @@ const parseArgs = (config = { __proto__: null }) => {
const index = StringPrototypeIndexOf(arg, '=');
const longOption = StringPrototypeSlice(arg, 2, index);
const optionValue = StringPrototypeSlice(arg, index + 1);
checkOptionUsage(longOption, optionValue, options, `--${longOption}`, strict);
checkOptionUsage(longOption, optionValue, options, `--${longOption}`, strict, allowPositionals);
storeOption(longOption, optionValue, options, result.values);
continue;
}

// Anything left is a positional
if (!allowPositionals) {
throw new ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL(arg);
}

ArrayPrototypePush(result.positionals, arg);
}

Expand Down
2 changes: 1 addition & 1 deletion test/dash.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ test("dash: when args include '-' used as positional then result has '-' in posi
const args = ['-'];
const expected = { values: { __proto__: null }, positionals: ['-'] };

const result = parseArgs({ args });
const result = parseArgs({ allowPositionals: true, args });

t.deepEqual(result, expected);
t.end();
Expand Down
52 changes: 48 additions & 4 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ test('handles short-option groups with "short" alias configured', () => {
test('Everything after a bare `--` is considered a positional argument', () => {
const args = ['--', 'barepositionals', 'mopositionals'];
const expected = { values: { __proto__: null }, positionals: ['barepositionals', 'mopositionals'] };
const result = parseArgs({ args });
const result = parseArgs({ allowPositionals: true, args });
assert.deepStrictEqual(result, expected, Error('testing bare positionals'));
});

Expand Down Expand Up @@ -127,7 +127,7 @@ test('args equals are passed `type: "string"`', () => {
test('when args include single dash then result stores dash as positional', () => {
const args = ['-'];
const expected = { values: { __proto__: null }, positionals: ['-'] };
const result = parseArgs({ args });
const result = parseArgs({ allowPositionals: true, args });
assert.deepStrictEqual(result, expected);
});

Expand Down Expand Up @@ -196,12 +196,12 @@ test('order of option and positional does not matter (per README)', () => {
const options = { foo: { type: 'string' } };
const expected = { values: { __proto__: null, foo: 'bar' }, positionals: ['baz'] };
assert.deepStrictEqual(
parseArgs({ args: args1, options }),
parseArgs({ allowPositionals: true, args: args1, options }),
expected,
Error('option then positional')
);
assert.deepStrictEqual(
parseArgs({ args: args2, options }),
parseArgs({ allowPositionals: true, args: args2, options }),
expected,
Error('positional then option')
);
Expand Down Expand Up @@ -288,6 +288,25 @@ test('excess leading dashes on options are retained', () => {
assert.deepStrictEqual(result, expected, Error('excess option dashes are retained'));
});

test('positional arguments are allowed by default in strict:false', () => {
const args = ['foo'];
const options = { };
const expected = {
values: { __proto__: null },
positionals: ['foo']
};
const result = parseArgs({ strict: false, args, options });
assert.deepStrictEqual(result, expected);
});

test('positional arguments may be explicitly disallowed in strict:false', () => {
const args = ['foo'];
const options = { };
assert.throws(() => { parseArgs({ strict: false, allowPositionals: false, args, options }); }, {
code: 'ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL'
});
});

// Test bad inputs

test('invalid argument passed for options', () => {
Expand Down Expand Up @@ -355,6 +374,31 @@ test('unknown option with explicit value', () => {
});
});

test('unexpected positional', () => {
const args = ['foo'];
const options = { foo: { type: 'boolean' } };
assert.throws(() => { parseArgs({ args, options }); }, {
code: 'ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL'
});
});

test('unexpected positional after --', () => {
const args = ['--', 'foo'];
const options = { foo: { type: 'boolean' } };
assert.throws(() => { parseArgs({ args, options }); }, {
code: 'ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL'
});
});

test('-- by itself is not a positional', () => {
const args = ['--foo', '--'];
const options = { foo: { type: 'boolean' } };
const result = parseArgs({ args, options });
const expected = { values: { __proto__: null, foo: true },
positionals: [] };
assert.deepStrictEqual(result, expected);
});

test('string option used as boolean', () => {
const args = ['--foo'];
const options = { foo: { type: 'string' } };
Expand Down
2 changes: 1 addition & 1 deletion test/short-option-groups.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ test('when pass full-config group of booleans then parsed as booleans', (t) => {
const options = { r: { type: 'boolean' }, f: { type: 'boolean' } };
const expected = { values: { __proto__: null, r: true, f: true }, positionals: ['p'] };

const result = parseArgs({ args, options });
const result = parseArgs({ allowPositionals: true, args, options });

t.deepEqual(result, expected);
t.end();
Expand Down