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

refactor!: restructure configuration to take options bag #63

Merged
merged 23 commits into from
Mar 2, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
697493f
feat: Restructure options API
aaronccasanova Feb 9, 2022
9e42db0
chore: Remove debug comments
aaronccasanova Feb 9, 2022
36ef68c
chore: Remove debug comments
aaronccasanova Feb 9, 2022
67c0b03
chore: Alias args to argv to introduce less changes
aaronccasanova Feb 9, 2022
082bec5
feat: Replace option with
Feb 12, 2022
0909008
docs: Update README to reflect updated implementation
Feb 12, 2022
042480b
chore: Revert args options to argv
Feb 12, 2022
e44c46c
docs: Update README from PR feedback
aaronccasanova Feb 24, 2022
4063751
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Feb 26, 2022
a50e940
chore: Reduce changes
aaronccasanova Feb 26, 2022
062bdc9
chore: Tidy up naming conventions
aaronccasanova Feb 26, 2022
28ae7b8
Merge branch 'main' into feat/restructure-options-api
bcoe Feb 27, 2022
4436ef6
Rename argv propery to args (#2)
shadowspawn Feb 28, 2022
dc58a4a
feat: Rename multiples to multiple
aaronccasanova Feb 28, 2022
14866a3
fix: Update union error formatting
aaronccasanova Feb 28, 2022
b0b71c4
feat: Guard against short options longer than 1 char
aaronccasanova Feb 28, 2022
92811f4
Update validators.js
aaronccasanova Feb 28, 2022
0d5a35e
chore: Add missing primordial
aaronccasanova Feb 28, 2022
ecf1fb0
fix: Replace for..of with primordial
aaronccasanova Mar 1, 2022
e0d0c33
fix: Update validator to use primoridial
aaronccasanova Mar 1, 2022
50d4f4d
fix: Clear up ambiguity around long and short option usage
aaronccasanova Mar 1, 2022
b41fd9b
fix: Update error formatting
aaronccasanova Mar 1, 2022
f533a98
Update index.js
aaronccasanova Mar 1, 2022
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
41 changes: 22 additions & 19 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,16 @@ process.mainArgs = process.argv.slice(process._exec ? 1 : 2)

----

## 💡 `util.parseArgs([argv][, options])` Proposal
## 💡 `util.parseArgs([config])` Proposal

* `argv` {string[]} (Optional) Array of argument strings; defaults
to [`process.mainArgs`](process_argv)
* `options` {Object} (Optional) The `options` parameter is an
* `config` {Object} (Optional) The `config` parameter is an
aaronccasanova marked this conversation as resolved.
Show resolved Hide resolved
object supporting the following properties:
* `withValue` {string[]} (Optional) An `Array` of argument
strings which expect a value to be defined in `argv` (see [Options][]
for details)
* `multiples` {string[]} (Optional) An `Array` of argument
strings which, when appearing multiple times in `argv`, will be concatenated
into an `Array`
* `short` {Object} (Optional) An `Object` of key, value pairs of strings which map a "short" alias to an argument; When appearing multiples times in `argv`; Respects `withValue` & `multiples`
* `argv` {string[]} (Optional) Array of argument strings; defaults
to [`process.mainArgs`](process_argv)
* `options` {Object} (Optional) An object describing the known options to look for in `argv`; `options` keys are the long names of the known options, and the values are objects with the following properties:

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A quirk of the current implementation is that a one character long option will be recognised as both a long option (e.g. --f) and a short option (e.g. -f). This is an existing quirk, and not introduced by this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see no issue with this unless we think there are cases where authors do NOT want to allow shorts for a given option. Is this a scenario you've encountered maintaining Commander? Or foresee other problems with this quirk?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Commander started out with dual short and long option names (needed to define both). Support for long-only options added in 2011. Support for short-only options added in 2020 (and more for completeness than because of demand).

* `type` {'string'|'boolean'} (Optional) Type of known option; defaults to `'boolean'`;
* `multiples` {boolean} (Optional) If true, when appearing one or more times in `argv`, results are collected in an `Array`
* `short` {string} (Optional) A single character alias for an option; When appearing one or more times in `argv`; Respects the `multiples` configuration
* `strict` {Boolean} (Optional) A `Boolean` on wheather or not to throw an error when unknown args are encountered
* Returns: {Object} An object having properties:
* `flags` {Object}, having properties and `Boolean` values corresponding to parsed options passed
Expand All @@ -106,7 +103,7 @@ const { parseArgs } = require('@pkgjs/parseargs');
const { parseArgs } = require('@pkgjs/parseargs');
const argv = ['-f', '--foo=a', '--bar', 'b'];
const options = {};
const { flags, values, positionals } = parseArgs(argv, options);
const { flags, values, positionals } = parseArgs({ argv, options });
// flags = { f: true, bar: true }
// values = { foo: 'a' }
// positionals = ['b']
Expand All @@ -117,9 +114,11 @@ const { parseArgs } = require('@pkgjs/parseargs');
// withValue
const argv = ['-f', '--foo=a', '--bar', 'b'];
const options = {
withValue: ['bar']
foo: {
type: 'string',
},
};
const { flags, values, positionals } = parseArgs(argv, options);
const { flags, values, positionals } = parseArgs({ argv, options });
// flags = { f: true }
// values = { foo: 'a', bar: 'b' }
// positionals = []
Expand All @@ -130,10 +129,12 @@ const { parseArgs } = require('@pkgjs/parseargs');
// withValue & multiples
const argv = ['-f', '--foo=a', '--foo', 'b'];
const options = {
withValue: ['foo'],
multiples: ['foo']
foo: {
type: 'string',
multiples: true,
},
};
const { flags, values, positionals } = parseArgs(argv, options);
const { flags, values, positionals } = parseArgs({ argv, options });
// flags = { f: true }
// values = { foo: ['a', 'b'] }
// positionals = []
Expand All @@ -144,9 +145,11 @@ const { parseArgs } = require('@pkgjs/parseargs');
// shorts
const argv = ['-f', 'b'];
const options = {
short: { f: 'foo' }
foo: {
short: 'f',
},
};
const { flags, values, positionals } = parseArgs(argv, options);
const { flags, values, positionals } = parseArgs({ argv, options });
// flags = { foo: true }
// values = {}
// positionals = ['b']
Expand Down
55 changes: 35 additions & 20 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

const {
ArrayPrototypeConcat,
ArrayPrototypeIncludes,
ArrayPrototypeSlice,
ArrayPrototypeSplice,
ArrayPrototypePush,
ObjectHasOwn,
ObjectEntries,
StringPrototypeCharAt,
StringPrototypeIncludes,
StringPrototypeIndexOf,
Expand All @@ -16,7 +16,10 @@ const {

const {
validateArray,
validateObject
validateObject,
validateString,
validateUnion,
validateBoolean,
} = require('./validators');

function getMainArgs() {
Expand Down Expand Up @@ -53,15 +56,14 @@ function getMainArgs() {
return ArrayPrototypeSlice(process.argv, 2);
}

function storeOptionValue(parseOptions, option, value, result) {
const multiple = parseOptions.multiples &&
ArrayPrototypeIncludes(parseOptions.multiples, option);
function storeOptionValue(options, option, value, result) {
shadowspawn marked this conversation as resolved.
Show resolved Hide resolved
const optionConfig = options[option] || {};

// Flags
result.flags[option] = true;

// Values
if (multiple) {
if (optionConfig.multiples) {
// Always store value in array, including for flags.
// result.values[option] starts out not present,
// first value is added as new array [newValue],
Expand All @@ -77,15 +79,25 @@ function storeOptionValue(parseOptions, option, value, result) {
}
}

const parseArgs = (
const parseArgs = ({
argv = getMainArgs(),
options = {}
) => {
} = {}) => {
validateArray(argv, 'argv');
validateObject(options, 'options');
for (const key of ['withValue', 'multiples']) {
if (ObjectHasOwn(options, key)) {
validateArray(options[key], `options.${key}`);
for (const [option, optionConfig] of ObjectEntries(options)) {
shadowspawn marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

for..of is never robust, because of Symbol.iterator; this should use ArrayPrototypeForEach(ObjectEntries(options), ([option, optionConfig]) => { instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! ecf1fb0

validateObject(optionConfig, `options.${option}`);

if (ObjectHasOwn(optionConfig, 'type')) {
validateUnion(optionConfig.type, `options.${option}.type`, ['string', 'boolean']);
}

if (ObjectHasOwn(optionConfig, 'short')) {
validateString(optionConfig.short, `options.${option}.short`);
shadowspawn marked this conversation as resolved.
Show resolved Hide resolved
}

if (ObjectHasOwn(optionConfig, 'multiples')) {
validateBoolean(optionConfig.multiples, `options.${option}.multiples`);
}
}

Expand Down Expand Up @@ -125,15 +137,19 @@ const parseArgs = (
}

arg = StringPrototypeCharAt(arg, 1); // short
if (options.short && options.short[arg])
arg = options.short[arg]; // now long!
for (const [option, optionConfig] of ObjectEntries(options)) {
if (optionConfig.short === arg) {
arg = option; // now long!
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const [option, optionConfig] of ObjectEntries(options)) {
if (optionConfig.short === arg) {
arg = option; // now long!
break;
}
}
const foundLong = ArrayPrototypeFind(ObjectEntries(options), ([, optionConfig]) => optionConfig.short === arg);
arg = foundLong?.[0] ?? StringPrototypeCharAt(arg, 1);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👌 ecf1fb0

// ToDo: later code tests for `=` in arg and wrong for shorts
} else {
arg = StringPrototypeSlice(arg, 2); // remove leading --
}

if (StringPrototypeIncludes(arg, '=')) {
// Store option=value same way independent of `withValue` as:
// Store option=value same way independent of `type: "string"` as:
// - looks like a value, store as a value
// - match the intention of the user
// - preserve information for author to process further
Expand All @@ -146,15 +162,15 @@ const parseArgs = (
} else if (pos + 1 < argv.length &&
!StringPrototypeStartsWith(argv[pos + 1], '-')
) {
// withValue option should also support setting values when '=
// `type: "string"` option should also support setting values when '='
// isn't used ie. both --foo=b and --foo b should work

// If withValue option is specified, take next position argument as
// value and then increment pos so that we don't re-evaluate that
// If `type: "string"` option is specified, take next position argument
// as value and then increment pos so that we don't re-evaluate that
// arg, else set value as undefined ie. --foo b --bar c, after setting
// b as the value for foo, evaluate --bar next and skip 'b'
const val = options.withValue &&
ArrayPrototypeIncludes(options.withValue, arg) ? argv[++pos] :
const val = options[arg] && options[arg].type === 'string' ?
argv[++pos] :
undefined;
storeOptionValue(options, arg, val, result);
} else {
Expand All @@ -163,7 +179,6 @@ const parseArgs = (
// save value as undefined
storeOptionValue(options, arg, undefined, result);
}

} else {
// Arguments without a dash prefix are considered "positional"
ArrayPrototypePush(result.positionals, arg);
Expand Down
Loading