Refactor parsing for readability and future expansion, and tighten short option group parsing#68
Refactor parsing for readability and future expansion, and tighten short option group parsing#68shadowspawn wants to merge 13 commits intopkgjs:mainfrom
Conversation
|
These updates are great @shadowspawn!! I'm honestly tempted to close out my PR in favor of this implementation. My goal was simply to swap out the conditionals with named utilities while introducing the smallest number of changes. That being said, I very much prefer the approach you took where each arg is assessed sequentially and the utility functions can stand on their own 👌 |
|
Ready for review. |
| * Determines if the option is expecting a value. | ||
| */ | ||
| function isExpectingValue(optionKey, options) { | ||
| return options && options.withValue && |
There was a problem hiding this comment.
| return options && options.withValue && | |
| return options && ArrayIsArray(options.withValue) && |
otherwise a truthy non-array could cause problems
There was a problem hiding this comment.
We call validateArray on withValue as soon as the options are received, so don't need to be paranoid about that. (Good advice in just local scope. The validation is outside the diff.)
There was a problem hiding this comment.
if we're sure it's an array when present, why would options be falsy?
| * @example | ||
| * isLongOption('-a) // returns false | ||
| * isLongOption('--foo) // returns true | ||
| * isLongOption('--foo=bar) // returns true | ||
| */ | ||
| function isLongOption(arg) { | ||
| return arg.length > 2 && StringPrototypeStartsWith(arg, '--'); |
There was a problem hiding this comment.
isLongOption('---') would also return true. Should we update this to test with regex?
e.g. /^--?\w+/.test(arg)
Or simply add && StringPrototypeCharAt(arg, 2) !== '-'
There was a problem hiding this comment.
isLongOption('---')would also returntrue.
Yes, this is an edge case but is as intended. See discussion in #7.
There was a problem hiding this comment.
Roger roger, thanks for the resource! Skimming the issue I'm in agreement it makes sense we limit the assumption here 👍
index.js
Outdated
| const onlyFlags = arg.slice(1, -1); | ||
| for (let index = 0; index < onlyFlags.length; index++) { | ||
| const optionKey = getOptionKey(StringPrototypeCharAt(onlyFlags, index)); |
There was a problem hiding this comment.
suggestion (non-blocking): This appears to be the only place in the parsing implementation referring to options as flags (outside of the results object). Perhaps we should update the name for consistency e.g. optionKeys, onlyOptionKeys, shortOptions, onlyShortOptions, etc.
There was a problem hiding this comment.
Ha! I knew that when I wrote it, but didn't try and think of something better! Fair call, will do. 😄
There was a problem hiding this comment.
Reworked wording and code:
const leadingShorts = StringPrototypeSlice(arg, 1, -1);
const allLeadingAreBoolean = ArrayPrototypeEvery(leadingShorts, (short) => {
Co-authored-by: Jordan Harband <ljharb@gmail.com>
|
|
||
| function storeOptionValue(parseOptions, option, value, result) { | ||
| function storeOptionValue(option, value, parseOptions, result) { | ||
| const multiple = parseOptions.multiples && |
There was a problem hiding this comment.
It seems like if we’re validating such that we know this is an array, why isn’t it always an array? Meaning, have it be an empty array here, rather than falsy/missing.
There was a problem hiding this comment.
(We currently validate, but not modify. Might be revamping configuration: #63)
|
@aaronccasanova, if you prefer this over #64, let's go with it? Same comment stands as in #64, which is that I'd advocate that we keep these helpers private. |
|
Yes, I prefer this implementation and agree the helpers should be private 👍 Wen't ahead and closed PR #64 |
|
I am working on rewrite for after #63 lands, so changed this to draft. |
|
Working on redo of this with new configuration bag of options. |
4 participants
Refactor heavily influenced by comments and code from @aaronccasanova in #64.
There is one functional change. A dubious option group with an option in the middle taking a value now returns the argument as a positional for BYO handling, and in strict mode this would throw. (The previous code was blindly expanding anything after a single
-.) This behaviour influenced by comments from @ljharb, and narrower implementation of the Open Group Utility Conventions.Separate out and extend tests for single
-and for short option groups.Apologies for the noise moving the tests, but something I badly wanted to do, and felt in scope for a big refactor!
First draft: no changes to unit tests. It works as before, as far as the tests went!
Second draft: separate out and extend
-unit testsThird draft: separate out and extend short option group tests