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.
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.
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.
gotcha, but the tests are static, yes? So the size will always be constant
There was a problem hiding this comment.
Unless there's 9 different tests for 1 case here ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
while the actual error is at the end
There was a problem hiding this comment.
What is unrelated option in this case? You mean flags?
There was a problem hiding this comment.
yeah sorry, so any update to their description or removal\addition will change lines amount
There was a problem hiding this comment.
right. Let's keep it for now, could change it later if needed
|
Ok in that case it is good (re |
|
Not clear if there is an action point left here @ev1stensberg , can you add review pls |
|
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