refactor!: restructure configuration to take options bag #63

aaronccasanova · 2022-02-09T01:53:36Z

Per the discussion in #45 this PR restructures the current options API where each option is configured in three separate list and instead allows options to be configured in a single object.

The goal being to make the API more intuitive for configuring options (e.g. short, withValue, and multiples) while creating a path forward for introducing more configuration options in the future (e.g. default).

Before:

parseArgs(process.argv.slice(2), {
  withValue: ['foo'],
  multiples: ['foo'],
  shorts: {
    f: 'foo',
  },
})

After:

parseArgs({
  args: process.argv.slice(2),
  options: {
    foo: {
      short: 'f',
      type: 'string', // defaults to 'boolean'
      multiple: true,
      // default: 'bar' - Future iteration
    }
  },
})

While the above mentioned discussion left off with a tentative interest in moving forward with the suggestion, I wanted to at least make an official PR with the proposed changes and have an implementation we could reference.

Note: All tests are passing and I've introduced a few new validation helpers to check the new structure

TODO: Assuming we decide to move forward with this approach

Update README examples

bcoe · 2022-02-11T13:44:26Z

I'm at least partially sold on this approach, especially if it would help us reach consensus on some other features .. such as my thinking that we could figure out a better API surface for strict if we configure as an options bag.

bcoe · 2022-02-11T15:13:21Z

@aaronccasanova I'd be interested in seeing the updated README if you have the time.

One open question for me, if an argument is a flag, would you just set withValue: false? is this confusing (@ljharb, @shadowspawn). I like that we could support strict with this approach, without adding an additional configuration -- I just wonder if withValue is the right variable name.

ljharb · 2022-02-11T15:16:51Z

It is a bit confusing; I’d prefer an enum of “value” or “flag” if that were possible.

aaronccasanova · 2022-02-11T16:38:31Z

@aaronccasanova I'd be interested in seeing the updated README if you have the time.

Will do 👍

if an argument is a flag, would you just set withValue: false? is this confusing

I agree this is a bit confusing. I was assuming users would omit withValue and leverage the zero config behavior where options without withValue are flags.

One alternative solution is to replace withValue with type (e.g. type: string | number | boolean). Not only does this allow users to explicitly define the option type, but provides more meaningful metadata to the parser. For example, If option was set to type: 'number' the parser could confidently capture negative numbers (-100) instead of expanding them as a short options group (-1 -0 -0) as mentioned here.

bcoe · 2022-02-12T15:30:07Z

One alternative solution is to replace withValue with type (e.g. type: string | number | boolean)

I don't hate that idea, but for MVP think starting with string and boolean would be best, in the future we could decide whether we should add additional types, or leave this to implementers.

aaronccasanova · 2022-02-12T17:51:24Z

for MVP think starting with string and boolean would be best

Agreed, I went ahead and replaced the withValue option with type, added validation checks for string|boolean, and updated the tests: 082bec5

aaronccasanova · 2022-02-12T18:28:45Z

@bcoe Updated the README to reflect the most recent implementation (with the type option): 0909008

README.md

bnb · 2022-02-18T21:43:48Z

Added the Merge into Node.js milestone, not because this needs to be merged by then but because IMO we should definitely figure this out - merge or close - before that IMO.

isaacs · 2022-02-23T19:54:51Z

I like this a lot. It also leaves the door open for things like description, options, conflictsWith, validate: () => Boolean, etc. And it's much more declarative/readable.

README.md

bcoe · 2022-02-26T16:04:01Z

I will make an effort to review this tomorrow ❤️

ljharb · 2022-02-28T20:05:20Z

index.js

-  for (const key of ['withValue', 'multiples']) {
-    if (ObjectHasOwn(options, key)) {
-      validateArray(options[key], `options.${key}`);
+  for (const [option, optionConfig] of ObjectEntries(options)) {


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

Thanks! ecf1fb0

ljharb · 2022-02-28T20:07:37Z

index.js

+        for (const [option, optionConfig] of ObjectEntries(options)) {
+          if (optionConfig.short === arg) {
+            arg = option; // now long!
+            break;
+          }
+        }


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);

👌 ecf1fb0

ljharb · 2022-02-28T20:08:17Z

validators.js

+
+function validateUnion(value, name, union) {
+  if (!union.includes(value)) {
+    throw new ERR_INVALID_ARG_TYPE(name, `[${union.join('|')}]`, value);


Suggested change

throw new ERR_INVALID_ARG_TYPE(name, `[${union.join('|')}]`, value);

throw new ERR_INVALID_ARG_TYPE(name, `[${StringPrototypeJoin(union, '|')}]`, value);

👌 e0d0c33

shadowspawn · 2022-02-28T20:41:53Z

Light comment: in terms of shim correctness, I am guessing the new validation helpers need to be outside validators.js which is a Node.js internal file?

Non-blocking. Interested in whether this is the case, but can be remedied in a separate PR.

Co-authored-by: John Gee <[email protected]>

index.js

shadowspawn

Only action items on my list are the two for loops noted by @ljharb
Approving assuming either fix those now, or open an issue to do later. 😄

aaronccasanova · 2022-03-01T03:15:50Z

Thanks everyone for the feedback and helping improve the robustness of the implementation!! I think the PR is in a really good spot and has laid a great foundation for future iterations. Here are some notable changes that have recently been introduced since the initial reviews:

The argv parser config has been renamed to args
The multiples option config has been renamed to multiple
Internal usages of the name option have been updated to longOption and shortOption respectively
Added validation to ensure a provided short option config is a single character (and test case)

shadowspawn

LGTM. Thanks for the enthusiasm and work @aaronccasanova

index.js

Co-authored-by: Jordan Harband <[email protected]>

aaronccasanova added 4 commits February 8, 2022 17:34

feat: Restructure options API

697493f

chore: Remove debug comments

9e42db0

chore: Remove debug comments

36ef68c

chore: Alias args to argv to introduce less changes

67c0b03

aaronccasanova mentioned this pull request Feb 10, 2022

fix(parser): handle negative numeric tokens #62

Closed

bakkot mentioned this pull request Feb 10, 2022

Improve API for configuring a high number of arguments #45

Closed

bcoe requested a review from darcyclarke February 11, 2022 13:30

feat: Replace option with

082bec5

Aaron Casanova added 2 commits February 12, 2022 10:21

docs: Update README to reflect updated implementation

0909008

chore: Revert args options to argv

042480b

aaronccasanova commented Feb 12, 2022

View reviewed changes

README.md Show resolved Hide resolved

bnb added this to the Merge into Node.js milestone Feb 18, 2022

This was referenced Feb 19, 2022

feat: Add strict mode to parser aaronccasanova/parseargs#1

Closed

Is strict actually a goal? #11

Closed

shadowspawn reviewed Feb 23, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

shadowspawn reviewed Feb 23, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

shadowspawn reviewed Feb 23, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

shadowspawn reviewed Feb 23, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

docs: Update README from PR feedback

e44c46c

shadowspawn mentioned this pull request Feb 26, 2022

Refactor parsing for readability and future expansion, and tighten short option group parsing #68

Closed

shadowspawn mentioned this pull request Feb 28, 2022

Rename argv propery to args aaronccasanova/parseargs#2

Merged

ljharb reviewed Feb 28, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

shadowspawn and others added 6 commits February 28, 2022 13:20

Rename argv propery to args (#2)

4436ef6

feat: Rename multiples to multiple

dc58a4a

fix: Update union error formatting

14866a3

feat: Guard against short options longer than 1 char

b0b71c4

Update validators.js

92811f4

Co-authored-by: John Gee <[email protected]>

chore: Add missing primordial

0d5a35e

shadowspawn reviewed Feb 28, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

shadowspawn approved these changes Feb 28, 2022

View reviewed changes

aaronccasanova added 4 commits February 28, 2022 18:14

fix: Replace for..of with primordial

ecf1fb0

fix: Update validator to use primoridial

e0d0c33

fix: Clear up ambiguity around long and short option usage

50d4f4d

fix: Update error formatting

b41fd9b

shadowspawn self-requested a review March 1, 2022 03:55

shadowspawn approved these changes Mar 1, 2022

View reviewed changes

ljharb approved these changes Mar 1, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

Update index.js

f533a98

Co-authored-by: Jordan Harband <[email protected]>

shadowspawn merged commit b412095 into pkgjs:main Mar 2, 2022

github-actions bot mentioned this pull request Mar 2, 2022

chore(main): release 0.4.0 #73

Merged

aaronccasanova mentioned this pull request Mar 3, 2022

feat: Add strict mode to parser #74

Merged

shadowspawn mentioned this pull request Mar 4, 2022

feat: short with value in same arg #69

Closed

aaronccasanova deleted the feat/restructure-options-api branch March 6, 2022 21:03

shadowspawn mentioned this pull request May 31, 2022

feat: add parsed meta-data to returned properties #129

Merged

shadowspawn mentioned this pull request Apr 9, 2023

parseArgs Type Definition nodejs/node#47487

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: restructure configuration to take options bag #63

refactor!: restructure configuration to take options bag #63

aaronccasanova commented Feb 9, 2022 •

edited

Loading

bcoe commented Feb 11, 2022

bcoe commented Feb 11, 2022

ljharb commented Feb 11, 2022

aaronccasanova commented Feb 11, 2022

bcoe commented Feb 12, 2022

aaronccasanova commented Feb 12, 2022

aaronccasanova commented Feb 12, 2022 •

edited

Loading

bnb commented Feb 18, 2022

isaacs commented Feb 23, 2022

bcoe commented Feb 26, 2022

ljharb Feb 28, 2022

aaronccasanova Mar 1, 2022

ljharb Feb 28, 2022

aaronccasanova Mar 1, 2022

ljharb Feb 28, 2022

aaronccasanova Mar 1, 2022

shadowspawn commented Feb 28, 2022 •

edited

Loading

This comment was marked as outdated.

shadowspawn left a comment

aaronccasanova commented Mar 1, 2022 •

edited

Loading

shadowspawn left a comment

	throw new ERR_INVALID_ARG_TYPE(name, `[${union.join('\|')}]`, value);
	throw new ERR_INVALID_ARG_TYPE(name, `[${StringPrototypeJoin(union, '\|')}]`, value);

refactor!: restructure configuration to take options bag #63

refactor!: restructure configuration to take options bag #63

Conversation

aaronccasanova commented Feb 9, 2022 • edited Loading

bcoe commented Feb 11, 2022

bcoe commented Feb 11, 2022

ljharb commented Feb 11, 2022

aaronccasanova commented Feb 11, 2022

bcoe commented Feb 12, 2022

aaronccasanova commented Feb 12, 2022

aaronccasanova commented Feb 12, 2022 • edited Loading

bnb commented Feb 18, 2022

isaacs commented Feb 23, 2022

bcoe commented Feb 26, 2022

ljharb Feb 28, 2022

Choose a reason for hiding this comment

aaronccasanova Mar 1, 2022

Choose a reason for hiding this comment

ljharb Feb 28, 2022

Choose a reason for hiding this comment

aaronccasanova Mar 1, 2022

Choose a reason for hiding this comment

ljharb Feb 28, 2022

Choose a reason for hiding this comment

aaronccasanova Mar 1, 2022

Choose a reason for hiding this comment

shadowspawn commented Feb 28, 2022 • edited Loading

This comment was marked as outdated.

shadowspawn left a comment

Choose a reason for hiding this comment

aaronccasanova commented Mar 1, 2022 • edited Loading

shadowspawn left a comment

Choose a reason for hiding this comment

aaronccasanova commented Feb 9, 2022 •

edited

Loading

aaronccasanova commented Feb 12, 2022 •

edited

Loading

shadowspawn commented Feb 28, 2022 •

edited

Loading

aaronccasanova commented Mar 1, 2022 •

edited

Loading