refactor(parseOptions): better handle errors#674
Merged
Conversation
freitagbr
requested changes
Feb 28, 2017
src/common.js
Outdated
| if (typeof opt === 'string') { | ||
| if (opt[0] !== '-') { | ||
| return options; | ||
| error("Must start the options string with a '-'"); |
Contributor
There was a problem hiding this comment.
error("Options string must start with a '-'")?
src/common.js
Outdated
| }); | ||
|
|
||
| if (!opt) return options; // defaults | ||
| var hasErrorOpts = typeof errorOptions === 'object'; |
Contributor
There was a problem hiding this comment.
I would be careful here, because typeof null === 'object'. This check exists elsewhere in this function (and in wrap).
Member
Author
|
@freitagbr PTAL |
freitagbr
requested changes
Mar 5, 2017
src/common.js
Outdated
| function parseOptions(opt, map, errorOptions) { | ||
| if (!map) error('parseOptions() internal error: no map given'); | ||
| // Validate input | ||
| if (typeof opt !== 'string' && !(opt instanceof Object)) { |
Contributor
There was a problem hiding this comment.
Object.create(null) instanceof Object === false, but Object.create(null) indeed creates an object. This is the correct way to check that something is an object:
typeof a === 'object' && a !== nullMaybe make a function for this.
src/common.js
Outdated
| } | ||
| }); | ||
| } else if (typeof opt === 'object') { | ||
| } else { // opt instanceof Option |
Member
Author
|
PTAL |
Codecov Report
@@ Coverage Diff @@
## master #674 +/- ##
==========================================
+ Coverage 94.33% 94.67% +0.33%
==========================================
Files 33 33
Lines 1183 1183
==========================================
+ Hits 1116 1120 +4
+ Misses 67 63 -4
Continue to review full report at Codecov.
|
freitagbr
approved these changes
Mar 8, 2017
Contributor
|
LGTM, just need to resolve the merge conflicts. |
For the case where `parseOptions()` is passed a string that does not start with a hyphen, we should reject this string instead of returning the same value. This has no change on any other test cases, and should not affect any commands since we are careful about what input we pass to `parseOptions()` This also adjust how we were handling error cases in the function. We were previously using two different calls to `common.error()`, one for if `errorOptions` is passed, and one without. This hurts readability, because it reads like "if we have errorOptions, then this is an error, and if we don't then it's also an error," instead of the more appropriate, "we reached an error case and should use errorOptions if it's available, otherwise we should signal an error without using it." We do not need to use `errorOptions` for the added call to `error()`, since this is an error which indicates misuse, and should not be recoverable. This adds a test case as well, for a line which was not previously covered in our tests. Partial fix for #671
This validates input at the top of `parseOptions()` for type correctness. This changes `typeof x === 'object'` to `x instanceOf Object` to improve robustness. Now we can safely use `errorOptions` knowing that if it has a truthy value, it will be an object, otherwise we should use defaults instead. The new tests ensure that we have 100% unit test coverage for this function.
The `isObject()` helper is used throughout common.js to reliably test if variables are JavaScript objects.
7607a94 to
649611a
Compare
Member
Author
|
Should be resolved now |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For the case where
parseOptions()is passed a string that does notstart with a hyphen, we should reject this string instead of returning
the same value. This has no change on any other test cases, and should
not affect any commands since we are careful about what input we pass to
parseOptions()This also adjust how we were handling error cases in the function. We
were previously using two different calls to
common.error(), one forif
errorOptionsis passed, and one without. This hurts readability,because it reads like "if we have errorOptions, then this is an error,
and if we don't then it's also an error," instead of the more
appropriate, "we reached an error case and should use errorOptions if
it's available, otherwise we should signal an error without using it."
We now reject invalid input at the top of the function. These are regular
Errors instead of calls tocommon.error()because they represent misuseof an internal function.
Added test cases bring this function to 100% test coverage.
Partial fix for #671