Make _.include/_.contains work with strings. #667

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@ianb

ianb commented Jul 11, 2012

This makes _.include() / _.contains() work with strings in addition to collections.

Note String.contains is a proposed method for Harmony: http://wiki.ecmascript.org/doku.php?id=harmony:string_extras – also it's really just another indexOf check, so it seems to make sense.

String.indexOf is present in Javascript 1.0, so a method identity check seemed unnecessary.

I wasn't sure where a doc change would go.

@ianb

This comment has been minimized.

Show comment
Hide comment
@ianb

ianb Jul 11, 2012

Also, this allows Javascript to naturally coerce the search item to a string. Arguably that's different than how _.include works with Arrays (since it uses === which does not allow this kind of coercion). This is the simplest implementation, but I'd also be comfortable with a type check on target (returning false if the target is not a string).

ianb commented Jul 11, 2012

Also, this allows Javascript to naturally coerce the search item to a string. Arguably that's different than how _.include works with Arrays (since it uses === which does not allow this kind of coercion). This is the simplest implementation, but I'd also be comfortable with a type check on target (returning false if the target is not a string).

@ianb

This comment has been minimized.

Show comment
Hide comment
@ianb

ianb Jul 11, 2012

I also can't figure out why this works with boxed strings, though it seems to (like new String('test')), since typeof new String('test') == 'object'

ianb commented Jul 11, 2012

I also can't figure out why this works with boxed strings, though it seems to (like new String('test')), since typeof new String('test') == 'object'

@kitcambridge

This comment has been minimized.

Show comment
Hide comment
@kitcambridge

kitcambridge Jul 11, 2012

Contributor

I also can't figure out why this works with boxed strings, though it seems to (like new String('test')), since typeof new String('test') == 'object'.

Use _.isString instead of typeof.

Contributor

kitcambridge commented Jul 11, 2012

I also can't figure out why this works with boxed strings, though it seems to (like new String('test')), since typeof new String('test') == 'object'.

Use _.isString instead of typeof.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jul 11, 2012

Contributor

Strings are array-like so that's why this works in modern browsers on the edge version of Underscore.

_.contains('test', 'e'); // => true
Contributor

jdalton commented Jul 11, 2012

Strings are array-like so that's why this works in modern browsers on the edge version of Underscore.

_.contains('test', 'e'); // => true
@ianb

This comment has been minimized.

Show comment
Hide comment
@ianb

ianb Jul 11, 2012

_.contains (in current underscore) didn't seem to work because of the test if (nativeIndexOf && obj.indexOf === nativeIndexOf) return obj.indexOf(target) != -1; where nativeIndexOf = ArrayProto.indexOf

Note:

_contains('test', 'es'); // => false

ianb commented Jul 11, 2012

_.contains (in current underscore) didn't seem to work because of the test if (nativeIndexOf && obj.indexOf === nativeIndexOf) return obj.indexOf(target) != -1; where nativeIndexOf = ArrayProto.indexOf

Note:

_contains('test', 'es'); // => false
Fix test for strings (to also catch boxed strings). And apparently th…
…e test related to boxed strings was backwards.
@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jul 11, 2012

Contributor

Note:
_.contains('test', 'es'); // => false

Yap, that's because being array-like means having a length and index properties like "hi"[1]; // i.
That said I dig me some ES6 sugar :P

Contributor

jdalton commented Jul 11, 2012

Note:
_.contains('test', 'es'); // => false

Yap, that's because being array-like means having a length and index properties like "hi"[1]; // i.
That said I dig me some ES6 sugar :P

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Aug 31, 2012

Owner

Closing as discussed in #668...

Owner

jashkenas commented Aug 31, 2012

Closing as discussed in #668...

@jashkenas jashkenas closed this Aug 31, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment