-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Add strict mode to parser #74
Changes from 26 commits
697493f
9e42db0
36ef68c
67c0b03
082bec5
0909008
042480b
10df671
e44c46c
4063751
a50e940
062bdc9
28ae7b8
e6fea72
ef06099
c228484
dd4f718
26df81c
a04d11c
ca05b43
a1c3544
fa0775b
5b89108
1d412da
a16fc7b
c4271a0
b6107bb
948ca9e
a2ea48d
38575cd
99355dd
e66a8b3
a4fc92a
12fd0e6
870cffe
694c75d
9b76d55
fcb8c8d
e56246c
b4e0daf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,16 +7,32 @@ class ERR_INVALID_ARG_TYPE extends TypeError { | |
} | ||
} | ||
|
||
class ERR_INVALID_OPTION_VALUE extends Error { | ||
constructor(message) { | ||
super(message); | ||
this.code = 'ERR_INVALID_OPTION_VALUE'; | ||
} | ||
} | ||
|
||
class ERR_INVALID_SHORT_OPTION extends TypeError { | ||
constructor(longOption, shortOption) { | ||
super(`options.${longOption}.short must be a single character, got '${shortOption}'`); | ||
this.code = 'ERR_INVALID_SHORT_OPTION'; | ||
} | ||
} | ||
|
||
class ERR_UNKNOWN_OPTION extends Error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise, if we can avoid adding any new errors it simplifies merging into Node.js. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see: #97 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, is it possible to namespace our error codes so we can provide the most accurate messages to users?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There appear to be error namespaces in node core: @bcoe @shadowspawn @ljharb What are your thoughts on using a namespace for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My preferences would be But yes, I can't see any reason not to use a namespace. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated unique |
||
constructor(message) { | ||
super(message); | ||
this.code = 'ERR_UNKNOWN_OPTION'; | ||
} | ||
} | ||
|
||
module.exports = { | ||
codes: { | ||
ERR_INVALID_ARG_TYPE, | ||
ERR_INVALID_SHORT_OPTION | ||
ERR_INVALID_OPTION_VALUE, | ||
ERR_INVALID_SHORT_OPTION, | ||
ERR_UNKNOWN_OPTION, | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,7 +34,9 @@ const { | |
|
||
const { | ||
codes: { | ||
ERR_INVALID_OPTION_VALUE, | ||
ERR_INVALID_SHORT_OPTION, | ||
ERR_UNKNOWN_OPTION, | ||
}, | ||
} = require('./errors'); | ||
|
||
|
@@ -73,8 +75,36 @@ function getMainArgs() { | |
} | ||
|
||
const protoKey = '__proto__'; | ||
function storeOptionValue(options, longOption, value, result) { | ||
const optionConfig = options[longOption] || {}; | ||
|
||
function storeOption({ | ||
strict, | ||
options, | ||
result, | ||
longOption, | ||
shortOption, | ||
optionValue, | ||
}) { | ||
const hasOptionConfig = ObjectHasOwn(options, longOption); | ||
|
||
aaronccasanova marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (strict) { | ||
const longOrShortOption = shortOption == null ? | ||
`long option '--${longOption}'` : | ||
`short option '-${shortOption}'`; | ||
|
||
if (!hasOptionConfig) { | ||
throw new ERR_UNKNOWN_OPTION(`Unknown ${longOrShortOption}`); | ||
} | ||
|
||
if (options[longOption].type === 'string' && optionValue == null) { | ||
throw new ERR_INVALID_OPTION_VALUE(`Missing value for ${longOrShortOption} with type:'string'`); | ||
} | ||
|
||
if (options[longOption].type === 'boolean' && optionValue != null) { | ||
throw new ERR_INVALID_OPTION_VALUE(`Unexpected value '${optionValue}' for ${longOrShortOption} with type:'boolean'`); | ||
} | ||
} | ||
|
||
const optionConfig = hasOptionConfig ? options[longOption] : {}; | ||
|
||
// Flags | ||
result.flags[longOption] = true; | ||
|
@@ -89,22 +119,24 @@ function storeOptionValue(options, longOption, value, result) { | |
// result.values[longOption] starts out not present, | ||
// first value is added as new array [newValue], | ||
// subsequent values are pushed to existing array. | ||
const usedAsFlag = value === undefined; | ||
const newValue = usedAsFlag ? true : value; | ||
const usedAsFlag = optionValue === undefined; | ||
const newValue = usedAsFlag ? true : optionValue; | ||
if (result.values[longOption] !== undefined) | ||
ArrayPrototypePush(result.values[longOption], newValue); | ||
else | ||
result.values[longOption] = [newValue]; | ||
} else { | ||
result.values[longOption] = value; | ||
result.values[longOption] = optionValue; | ||
} | ||
} | ||
|
||
const parseArgs = ({ | ||
args = getMainArgs(), | ||
strict = false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's default this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Started with a naive approach to get an understanding of how this change affects our existing tests:
I'm committing early for visibility but also I will likely need assistance updating/validating these tests. At first glance of the diff I'm wondering if we can clear things up by organizing these tests in a cc @shadowspawn There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the naive approach to existing tests is fine in the first instance. If test passes as is then no need to change. If it was testing a zero-config behaviour then it needs (I would like to expand the end-to-end tests a bit, or at least review the coverage, but not got to that.) |
||
options = {} | ||
} = {}) => { | ||
validateArray(args, 'args'); | ||
validateBoolean(strict, 'strict'); | ||
validateObject(options, 'options'); | ||
ArrayPrototypeForEach( | ||
ObjectEntries(options), | ||
|
@@ -160,7 +192,14 @@ const parseArgs = ({ | |
// e.g. '-f', 'bar' | ||
optionValue = ArrayPrototypeShift(remainingArgs); | ||
} | ||
storeOptionValue(options, longOption, optionValue, result); | ||
storeOption({ | ||
strict, | ||
options, | ||
result, | ||
longOption, | ||
shortOption, | ||
optionValue, | ||
}); | ||
continue; | ||
} | ||
|
||
|
@@ -191,7 +230,14 @@ const parseArgs = ({ | |
const shortOption = StringPrototypeCharAt(arg, 1); | ||
const longOption = findLongOptionForShort(shortOption, options); | ||
const optionValue = StringPrototypeSlice(arg, 2); | ||
storeOptionValue(options, longOption, optionValue, result); | ||
storeOption({ | ||
strict, | ||
options, | ||
result, | ||
longOption, | ||
shortOption, | ||
optionValue, | ||
}); | ||
continue; | ||
} | ||
|
||
|
@@ -203,7 +249,7 @@ const parseArgs = ({ | |
// e.g. '--foo', 'bar' | ||
optionValue = ArrayPrototypeShift(remainingArgs); | ||
} | ||
storeOptionValue(options, longOption, optionValue, result); | ||
storeOption({ strict, options, result, longOption, optionValue }); | ||
continue; | ||
} | ||
|
||
|
@@ -212,7 +258,7 @@ const parseArgs = ({ | |
const index = StringPrototypeIndexOf(arg, '='); | ||
const longOption = StringPrototypeSlice(arg, 2, index); | ||
const optionValue = StringPrototypeSlice(arg, index + 1); | ||
storeOptionValue(options, longOption, optionValue, result); | ||
storeOption({ strict, options, result, longOption, optionValue }); | ||
continue; | ||
} | ||
|
||
|
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.
As @cjihrig suggests elsewhere, could se use:
To avoid needing to introduce a new error to
errors.js
?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.
Here's an early WIP with all the strict error cases updated to use
ERR_INVALID_ARG_VALUE
: 948ca9eI could use some suggestions for the error messages as the new message constraint of
ERR_INVALID_ARG_VALUE
and my current implementation doesn't read entirely accurate:Really I'm hung up on the word
property
overoption
(e.g.The option --foo must...
), but not sure if I'm overthinking it. Just want the message to be super clear to users.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.
Similarly, the current unknown option message isn't entirely helpful to users e.g.
$ node unknown-option.js --foo TypeError: The property option.foo must be configured, Received 'undefined'
Any suggestions to make this message more user-facing? e.g.
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.
As a user I would expect the message to be "Unknown option --foo".
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.
Agreed, that was the original message before the rework with the addition of dynamism for long and short options e.g.
Unknown long option --foo
andUnknown short option -f
. In an effort to reduce friction merging into Node I'm trying to find an equally helpful message within the constraints ofERR_INVALID_ARG_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.
I don't think the format for
ERR_INVALID_ARG_VALUE
is a particularly good match for our use here where we are referring to options on the command line, rather than arguments to a javascript function. (Valiant try though!)I see the node 18.x branch has updated code for the error which inserts property/argument depending on the name, but still not a good match for our option errors.
https://github.com/nodejs/node/blob/ec5a359ffd38b6f56a456a0b6a482b5167fbfce1/lib/internal/errors.js#L1246
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.
Reverted to custom error classes and updated the error messages per: #74 (comment)
Examples based on the PR description prompts:
type:'string'
used like a boolean option e.g. lone--string
type:'boolean'
used like a string option e.g.--boolean=foo
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.
👍
My only minor wording musing is for the "Option of
type:'string'
used like a boolean option". With the option I used, I was briefly confusing by the long option name reused as the option argument name. I wonder aboutvalue
orarg
? But haven't convinced myself they are better, unless you really like one, stick with what you have!The reuse does tend to work semantically. I picked a long one out of the node options to try:
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.
Good point! That example is excessively long. I think I'll use
value
for consistency with theparseArgs
results object.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.
870cffe