Fix the comparison for the args/options warnings #394
Conversation
lib/utils.js
Outdated
| // We need to be sure that it's not _both_ an options and args object: | ||
| // If the args keys and options keys are not the same length, | ||
| // then it contains both arguments and options, and that's not good. | ||
| if (argKeys.length !== optionKeysInArgs.length) { |
There was a problem hiding this comment.
Sorry @jlomas-stripe, I think I'm a bit thick, but I've been looking at this for a couple minutes now and still don't quite understand why this condition is written like this.
Is the idea that we're checking for the presence of optionKeysInArgs unless args and options are the same object? If it's something like that, is there anyway that we could rewrite this to be a little more easily readable (even if we have to make it a bit longer by making it two separate conditions)? I think it might help a bit for people to wrap their head around the logic when they see it.
There was a problem hiding this comment.
We're not checking for the presence of options keys, we're checking for a difference between the number of options keys and the number of args keys.
If all the args keys are options, then this is fine and it's an options object so we can ignore it; if there's some of both, then we need to warn.
I.e., if we have 3 argKeys but only 2 optionKeysInArgs then there's 1 non-options key in there - i.e., an argument - so we need to warn. If we have 3 and 3, then this is an options object, so it's fine and we'll return {} here and we don't need to warn.
Does that help?
There was a problem hiding this comment.
More explicitly, this previously tested for "are there options keys in here?" but that turned out to be wrong since this can be an options object - which obviously has options keys. :)
...Maybe this would be more clear?
if (optionsKeysInArgs.length > 0 && argKeys.length !== optionsKeysInArgs.length) {}...?
There was a problem hiding this comment.
Cool, thanks for the explanation!
Yeah, actually would you mind expanding to the two conditions you have above? Also, can we write a more elaborate comment to hopefully help with clarity? Maybe something like this:
In some cases options may be the provided as the first argument. Here we're detecting a case where there are two distinct arguments (the first being args and the second options) and with known option keys in the first so that we can warn the user about it.
There was a problem hiding this comment.
That's a way better comment. :)
Done and done. :)
|
Nice work as usual @jlomas-stripe! I'll cut a release. |
|
Awesome! Thanks for helping make it more clear! :) I did just add another PR (#395) that we might want to sneak into the release, assuming it makes sense. |
Fixes #393. The initial change (#306) didn't properly handle the case where the object was all
options- it wasconsole.warning in that case - but now it handles it properly.