Skip to content
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.

Sign up for GitHub

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

Fix the comparison for the args/options warnings #394

Merged
merged 2 commits into from
Oct 4, 2017
Merged

Fix the comparison for the args/options warnings #394

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
308f29c
Fix the comparison for the args/options warnings
jlomas-stripe Oct 3, 2017
26a93b1
Make condition more explicit and update comment
jlomas-stripe Oct 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ var utils = module.exports = {
return OPTIONS_KEYS.indexOf(key) > -1;
});

if (optionKeysInArgs.length > 0) {
// 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) {
Copy link
Contributor

@brandur-stripe brandur-stripe Oct 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jlomas-stripe jlomas-stripe Oct 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {}

...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a way better comment. :)

Done and done. :)

console.warn(
'Stripe: Options found in arguments (' + optionKeysInArgs.join(', ') + '). Did you mean to pass an options ' +
'object? See https://github.com/stripe/stripe-node/wiki/Passing-Options.'
Expand Down
38 changes: 25 additions & 13 deletions test/utils.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,32 +101,32 @@ describe('utils', function() {
it('handles an empty list', function() {
expect(utils.getDataFromArgs([])).to.deep.equal({});
});
it('handles an list with no object', function() {
it('handles a list with no object', function() {
var args = [1, 3];
expect(utils.getDataFromArgs(args)).to.deep.equal({});
expect(args.length).to.equal(2);
});
it('ignores a hash with only options', function() {
var args = [{api_key: 'foo'}];
expect(utils.getDataFromArgs(args)).to.deep.equal({});
expect(args.length).to.equal(1);

handleConsoleWarns(function() {
expect(utils.getDataFromArgs(args)).to.deep.equal({});
expect(args.length).to.equal(1);
}, function(message) {
throw new Error('Should not have warned, but did: ' + message);
});
});
it('throws an error if the hash contains both data and options', function() {
it('warns if the hash contains both data and options', function() {
var args = [{foo: 'bar', api_key: 'foo', idempotency_key: 'baz'}];

// Hack to make sure we're logging to console.warn here:
var _warn = console.warn;

console.warn = function(message) {
handleConsoleWarns(function() {
utils.getDataFromArgs(args);
}, function(message) {
expect(message).to.equal(
'Stripe: Options found in arguments (api_key, idempotency_key).' +
' Did you mean to pass an options object? See https://github.com/stripe/stripe-node/wiki/Passing-Options.'
);
};

utils.getDataFromArgs(args);

console.warn = _warn;
});
});
it('finds the data', function() {
var args = [{foo: 'bar'}, {api_key: 'foo'}];
Expand Down Expand Up @@ -260,3 +260,15 @@ describe('utils', function() {
});
});
});

function handleConsoleWarns(doWithShimmedConsoleWarn, onWarn) {
// Shim `console.warn`
var _warn = console.warn;

console.warn = onWarn;

doWithShimmedConsoleWarn();

// Un-shim `console.warn`
console.warn = _warn;
}