-
Notifications
You must be signed in to change notification settings - Fork 9
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
refactor!: restructure configuration to take options bag #63
Conversation
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 |
@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 |
It is a bit confusing; I’d prefer an enum of “value” or “flag” if that were possible. |
Will do 👍
I agree this is a bit confusing. I was assuming users would omit One alternative solution is to replace |
I don't hate that idea, but for MVP think starting with |
Agreed, I went ahead and replaced the |
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. |
I like this a lot. It also leaves the door open for things like |
I will make an effort to review this tomorrow ❤️ |
index.js
Outdated
for (const key of ['withValue', 'multiples']) { | ||
if (ObjectHasOwn(options, key)) { | ||
validateArray(options[key], `options.${key}`); | ||
for (const [option, optionConfig] of ObjectEntries(options)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! ecf1fb0
index.js
Outdated
for (const [option, optionConfig] of ObjectEntries(options)) { | ||
if (optionConfig.short === arg) { | ||
arg = option; // now long! | ||
break; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 ecf1fb0
validators.js
Outdated
|
||
function validateUnion(value, name, union) { | ||
if (!union.includes(value)) { | ||
throw new ERR_INVALID_ARG_TYPE(name, `[${union.join('|')}]`, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new ERR_INVALID_ARG_TYPE(name, `[${union.join('|')}]`, value); | |
throw new ERR_INVALID_ARG_TYPE(name, `[${StringPrototypeJoin(union, '|')}]`, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 e0d0c33
Light comment: in terms of shim correctness, I am guessing the new validation helpers need to be outside Non-blocking. Interested in whether this is the case, but can be remedied in a separate PR. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the enthusiasm and work @aaronccasanova
Co-authored-by: Jordan Harband <ljharb@gmail.com>
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
, andmultiples
) while creating a path forward for introducing more configuration options in the future (e.g.default
).Before:
After:
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