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.

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 #265 #361

Merged
merged 1 commit into from Feb 9, 2015
Merged

Fix #265 #361

merged 1 commit into from Feb 9, 2015

Conversation

@gregglind
Copy link
Contributor

@gregglind gregglind commented Feb 7, 2015

  1. We didn't throw on 'too many args' before. Should we now? If so, what?
  2. (Do docs run as tests, if so how?)
  3. (Tests inbound. WIP mostly to claim the bug :) )
@gregglind gregglind force-pushed the gregglind:b265-keys-object branch from 4db3314 to 0baa6f1 Feb 8, 2015
@gregglind
Copy link
Contributor Author

@gregglind gregglind commented Feb 8, 2015

See #265

}
} else {
keys = Array.prototype.slice.call(arguments);
}

This comment has been minimized.

@keithamus

keithamus Feb 8, 2015
Member

It might read clearer to use the _.type function:

if (_.type(keys) === 'object') {
    keys = Object.keys(keys);
} else if (arguments.length > 1) {
    keys = Array.prototype.slice.call(arguments);
}

if (keys instanceof Array || keys instanceof Object) {
if (arguments.length > 1) {
throw (new Error(mixedArgsMsg));

This comment has been minimized.

@keithamus

keithamus Feb 8, 2015
Member

Not entirely sure about throwing errors here. Think this may be overcomplicating the method a little is all.

This comment has been minimized.

@gregglind

gregglind Feb 8, 2015
Author Contributor

The 'throw' was mentioned in the bug, so I added it. I would think it would be very strange to do keys(Array, string...) as dev, but it could happen by accident. Low-cost to check :)

@gregglind gregglind force-pushed the gregglind:b265-keys-object branch from 0baa6f1 to 2b46e95 Feb 8, 2015
@gregglind
Copy link
Contributor Author

@gregglind gregglind commented Feb 8, 2015

Went to switch based, using _.type. I think it's very explicit now, for good or ill.

? keys
: Array.prototype.slice.call(arguments);
, ok = true
, mixedArgsMsg = "keys: if first argument is instanceof Array|Object, must be only argument."

This comment has been minimized.

@keithamus

keithamus Feb 8, 2015
Member

I think this error needs rephrasing, maybe to something like 'keys must be given single argument of Array, Object, or multiple String arguments'

This comment has been minimized.

@gregglind

gregglind Feb 9, 2015
Author Contributor

Rephased to:

    var mixedArgsMsg = 'keys must be given single argument of Array|Object|String, or multiple String arguments'

I think that is accurate. I do wonder if we should emphasize the 'multiple arguments' part, because that is what triggered the error.

- `.keys(object)n => .keys(Object.keys(Object)`
- added exceptions for 'if first arg is non-string, then it must be only
arg.  => `.keys(Array|Object, ...)`

Warning:  `Object.keys` must exist on systems to use this functionality.
@gregglind gregglind force-pushed the gregglind:b265-keys-object branch from 2b46e95 to 4f40b37 Feb 9, 2015
@keithamus
Copy link
Member

@keithamus keithamus commented Feb 9, 2015

I'd say this is good to go. Let's get it merged in. 👍

keithamus added a commit that referenced this pull request Feb 9, 2015
Fix #265
@keithamus keithamus merged commit 5753ecb into chaijs:master Feb 9, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@keithamus keithamus mentioned this pull request Feb 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants