Add proper Object function matches

[jridgewell]

This takes _.matches and turns it into a proper Object Function (takes the object as the first parameter). This should satisfy the performance problems seen in jashkenas/backbone#3487.

I think it’s pretty apparent we jumbled the arguments originally. See the need to special case Backbone.Model#matches instead of auto proxying like the other _ methods. We even have to special case Collection#matches so we don’t kill performance doing simple wheres.

The legacy _.matches(obj) predicate matcher has been extracted into _.matcher. _.matches will return the predicate matcher if only one argument is provided. This legacy support should be removed in v2.0.

[jridgewell]

Note: I'm intentionally not assigning to object here to avoid an arguments deopt.

[jdalton]

lodash has _.isMatch it'd be nice to align to that and avoid overloading _.matches.

[jridgewell]

Sorry, I meant to link #1985 as well.

I overloaded _.matches because the current functionality isn't implied by the name (I think _.matcher fits it better). I'm not tied to it, and I think getting this (or _.isMatch) merged is more important than my dislike of the current name.

[jashkenas]

Very nice.

_.matches and _.matcher are nice names for it.

[jdalton]

Way to be a team player :(

_.matches already exists, it seems odd to change the behavior now. And most methods in Underscore that return a boolean are prefixed with is.

[megawac]

Yeah, I agree with @jdalton I think it makes sense for this to be matches (old behaviour) and isMatch for bc.

This will actually break some suggestions I've made on SO and the issues as matches is not gaurded.

[arthurvr]

Way to be a team player.

Ya :(

[jridgewell]

_.matches already exists. It seems odd to change the behavior now.

But it would make the Backbone method ugly. Model#isMatch isn't as nice as Model#matches.

And most methods in Underscore that return a boolean are prefixed with is.

Could be aliased?

This will actually break some suggestions I've made on SO and the issues as matches is not gaurded.

We can add a guard here, and remove that with v2.0 as well.

---

I just really dislike having _.matches return a function... 😄

[megawac]

I feel like that is more effort than its worth: I'd say just leave matches be

[jdalton]

I just really dislike having _.matches return a function... 😄

I've gotten used to it. It makes sense since it produces a function that checks if an object matches the source. This change feels inconsistent since there's .isEqual and not .equals. And again the overloading is gross, sometimes returning a function or a boolean is yuck, @michaelficarra is rolling over in his seat.

[jridgewell]

@megawac: how were you using _.matches as an iteratee? _.filter(arr, _.matches) doesn't make sense.

@jashkenas: Thoughts on overloading?

[megawac]

@jridgewell it was something like

var obj = {a: 1};
var hasMatch = _.chain([{b: 1}, 'a']).map(_.iteratee).any((matcher) => matcher(obj)).value();

I don't recall exactly what the code was

[jashkenas]

_.matches was a crappy name for what it was doing — it shouldn't have been added with that name in the first place. Mea culpa.

_.matches(options, {async: true})    // => true

Reads nicely.

The overloading is certainly gross, but can be removed over time. Leaving the function named poorly would be worse in the long term.

[megawac]

@jashkenas I disagree, a poorly named function is much better than changing the contract. This will force some users using matches to update their code for no particularly good reason

[jdalton]

_.matches was a crappy name for what it was doing

Naming is hard and it can be seen to work as both so _.matches is fine as it was before this PR. For the new functionality _.isMatch better follows the naming conventions in Underscore.

_.matches(options, {async: true})    // => true

Reads nicely.

I'd argue that it doesn't read as nicely because devs are used to a different behavior (with a single arg).

_.matches(source);
_.matches(object, source);

Now, sometimes the first _.matches argument is the source and others it's the object which is clunky.
Something like _.isMatch makes it clear you're performing a check and getting a boolean result.

[braddunbar]

Yea, returning a function or a boolean with an optional first argument, not second, is about an confusing as it gets. If we dislike the name _.matches then let's alias and deprecate it. I don't think a bad name is a good reason to add a breaking change with completely new functionality.

[jridgewell]

I don't think a bad name is a good reason to add a breaking change with completely new functionality.

I don't think any breaking change was introduced. @megawac mentioned guarded behavior but you would never use _.matcher as an iteratee, only it's returned method. I also searched his StackOverflow answers (sorry that's kind of stalker-y) and only found one answer pertaining to _.matches, but it isn't affected by this. And if this is really the concern, we can add a guard flag until the legacy support is removed.

The optional first arg is clunky, but it'll go away in the next major (hopefully) release.

[megawac]

@jridgewell here's a (currently non working) example of something that would break with this

var isBobOrAlice = _.partial(_.anyPass, _.map([{name: 'Bob'}, {name: 'Alice'}], _.matches));
isBobOrAlice({name: 'Bob'})

#2008 (comment)

[jridgewell]

Damn. I'll work a guard PR.

[megawac]

Yea, returning a function or a boolean with an optional first argument, not second, is about an confusing as it gets. If we dislike the name _.matches then let's alias and deprecate it. I don't think a bad name is a good reason to add a breaking change with completely new functionality.

👍

The current way this is being implemented wastes LOC, simplicity and introduces breaking risks for a name. Just implement _.isMatch instead 😪

[jashkenas]

Alright then — isMatch, and matcher, with matches aliased and deprecated, and removed from the docs.

@jridgewell — you got this?

[jridgewell]

Yah, I'll get a new PR tonight.

Can we get a 2.0 milestone with _.isMatch aliased to _.matches (and possibly _.isEqual -> _.equals)?

[jashkenas]

Ah, I was thinking about sooner than that ;) I'll get it.

Re: 2.0 — nah. A little naming jiggery-pokery isn't a good enough reason to start rolling that ball down the hill.

[danfinlay]

Model#isMatch isn't as nice as Model#matches.

How about pick the grammatically correct isMatching?