-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Issue 249 fixed and other enums refactored #264
Conversation
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
Hi @ev1stensberg |
Maybe this is apart of the 0CJS strategy, don't really know. Anyways, I'm good with this, the schema validation will pick up on the missing things I assume. |
expect(stderr[4]).toContain("configuration['info-verbosity'] should be one of these:"); | ||
expect(stderr[5]).toContain("\"none\" | \"info\" | \"verbose\""); | ||
expect(stderr[stderr.length - 3]).toContain("Invalid values:"); | ||
expect(stderr[stderr.length - 2]).toContain(" Argument: info-verbosity, Given: \"false\", Choices: \"none\", \"info\", \"verbose\""); |
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.
Maybe this could be done without slicing at the end of the array? If you already know the size of it?
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.
Total length will vary because adding more options into yargs.options
will change the total length as invalid options do print out the whole usage info, but i know that -3 and -2 will be error specific values that i want to check
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.
gotcha, but the tests are static, yes? So the size will always be constant
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.
Unless there's 9 different tests for 1 case here ?
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.
Size will change if anyone update unrelated option here and it will require this test case update.
I mean this unrelated options will increase/decrease the output line count as all of them are printed on invalid option
Lines 65 to 92 in 73fdf7d
silent: { | |
type: "boolean", | |
describe: "Prevent output from being displayed in stdout" | |
}, | |
json: { | |
type: "boolean", | |
alias: "j", | |
describe: "Prints the result as JSON." | |
}, | |
progress: { | |
type: "boolean", | |
describe: "Print compilation progress in percentage", | |
group: BASIC_GROUP | |
}, | |
color: { | |
type: "boolean", | |
alias: "colors", | |
default: function supportsColor() { | |
return require("supports-color"); | |
}, | |
group: DISPLAY_GROUP, | |
describe: "Enables/Disables colors on the console" | |
}, | |
"sort-modules-by": { | |
type: "string", | |
group: DISPLAY_GROUP, | |
describe: "Sorts the modules list by property in module" | |
}, |
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.
while the actual error is at the end
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.
What is unrelated option in this case? You mean flags?
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.
yeah sorry, so any update to their description or removal\addition will change lines amount
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.
right. Let's keep it for now, could change it later if needed
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.
true, thank you!
Ok in that case it is good (re |
Not clear if there is an action point left here @ev1stensberg , can you add review pls |
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.
ltgm, thanks @EugeneHlushko . Hero. ❤️
haha thank you ❤️ |
What kind of change does this PR introduce?
Validation in proper place
Did you add tests for your changes?
yes
If relevant, did you update the documentation?
Summary
FIxes #249 and small refactor of similar enum string options
Does this PR introduce a breaking change?
Other information