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

Consider renaming (or aliasing) `match_array` to `contain_exactly` #398

Closed
myronmarston opened this issue Dec 23, 2013 · 7 comments
Closed

Consider renaming (or aliasing) `match_array` to `contain_exactly` #398

myronmarston opened this issue Dec 23, 2013 · 7 comments

Comments

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Dec 23, 2013

In #119 when we added the new expect syntax and chose to not support the operator matchers for that syntax, we had to come up with a name for array.should =~ other_array, and decided on match_array as the name after a rather lengthy discussion. (See #119 (comment) and following for the full conversation).

Working on #393 and seeing how well the matcher name matches the description output for almost all matchers, I realized that match_array's description hints at a better name. The description is contain exactly <pretty printed array>. During a skype call with @xaviershay about #393 he mentioned some confusion about what the exact semantics of match_array are (since match is such a generic term) and I'm thinking that contain_exactly is a better name, e.g.:

expect(results).to contain_exactly(1, 2, 3)

Compare to:

expect(results).to match_array([1, 2, 3])

The fact that with match_array you have to wrap the elements in [ ] has always bugged me a bit (as it makes it more syntactically noisy), but I've always felt like that since match_array has the word "array" in it, the argument should be an array. With contain_exactly it doesn't give that connotation and I think it reads better w/o the array square brackets.

If we do move in this direction, there's a few open questions:

  • Do we deprecate match_array and keep only contain_exactly? I'm on the fence about this. On the one had, there's no significant maintenance cost to keeping both. On the other hand, it'll probably never be less painful than it is now to deprecate match_array: we have 2.99 for the migration and transpec will likely support the conversion out of the box. However, I've gotten some feedback on twitter that some users feel like we are renaming/deprecating things too quickly (although, that was more about 2.14 than 2.99, and I think 2.99's deprecation support is significantly more user-friendly)
  • If we do keep match_array, will it be confusing that match_array accepts a single array arg where as contain_exactly accepts them splatted out?

/cc @xaviershay @JonRowe @samphippen @soulcutter @alindeman

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Dec 23, 2013

I like contain_exactly, but I'm on the fence about deprecating it, some people might prefer match_array, especially if you're passing in a variable. e.g. expect(something.result).to match_array of_things

@myronmarston
Copy link
Member Author

@myronmarston myronmarston commented Dec 23, 2013

some people might prefer match_array, especially if you're passing in a variable. e.g. expect(something.result).to match_array of_things

This works pretty OK:

expect(something.result).to contain_exactly(*things)

But I'm on the fence about the deprecation, too.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Dec 23, 2013

My concern is that requiring people use expect(something.result).to contain_exactly(*things) will alienate people...

@soulcutter
Copy link
Member

@soulcutter soulcutter commented Dec 23, 2013

Newer rubyists aren't as comfortable with the splat operator. I like contains_exactly but I would keep the old syntax around, and probably just document that it's identical.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Dec 23, 2013

Specifying the splat difference of course ;)

@soulcutter
Copy link
Member

@soulcutter soulcutter commented Dec 23, 2013

Indeed

myronmarston added a commit that referenced this issue Dec 24, 2013
@myronmarston
Copy link
Member Author

@myronmarston myronmarston commented Dec 24, 2013

OK, I've implemented this in 1f3ab8d, which I've pushed to #393 since I don't want to deal with merge conflicts. I'll close this here to concentrate discussion over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants