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

Add Must.prototype.permutationOf #16

Merged
merged 1 commit into from
May 28, 2014

Conversation

bajtos
Copy link
Contributor

@bajtos bajtos commented May 13, 2014

This is similar to have members from chai.

It would be nice to have include members or is subset of too, but that can be implemented in a different patch.

@bajtos
Copy link
Contributor Author

bajtos commented May 15, 2014

Hey @moll, what's your take on this pull request? Is it something you are willing to accept into the project?

@moll
Copy link
Owner

moll commented May 15, 2014

Hey, Miroslav!

Thank you very much for taking the time to share this! Absolutely! I simply hadn't had the moment to handle this yet.

Would you do me two quick favors please?

  • Remove the semicolon from line 531.
  • Name it just members. I understand equate might make sense for those coming from C#, but Must.js already has these equal, eql matchers and I'm afraid equate might be a little too similar to them. :-)

You can use Git's commit --amend feature to add those additions.
If you don't have the time, let me know and I'll do those myself before merging. ;-)

Thank you again!

@bajtos
Copy link
Contributor Author

bajtos commented May 16, 2014

@moll You are welcome.

I have amended the commit per your instructions, also added diffable: true flag. I am not sure what it does, but eql sets it to true too, so I thought it would be better to do the same.

Let me know if there is anything else to improve.

@bajtos bajtos changed the title Add Must.prototype.equate Add Must.prototype.members May 21, 2014
@bajtos
Copy link
Contributor Author

bajtos commented May 21, 2014

@moll ping. is there anything else to fix before this can be merged and released?

bajtos pushed a commit to strongloop/generator-loopback that referenced this pull request May 21, 2014
The proper implementation will be available when the pull request
moll/js-must#16 is merged and released.
@moll
Copy link
Owner

moll commented May 21, 2014

Nah, sorry, you've been perfect. Thank you! It's been me. :-)
I'll get some sleep and merge and release it tomorrow for you!

I have amended the commit per your instructions, also added diffable: true flag. I am not sure what it does, but eql sets it to true too, so I thought it would be better to do the same.

Diffable is a mark for a test runner (like Mocha) to show a "diff" view side by side. Makes sense for members indeed! Thanks.

@moll
Copy link
Owner

moll commented May 22, 2014

Hey again, @bajtos.

So, I went about merging this and then realized members might actually be either an ambiguous name (because it's not clear if it's a subset matcher or not), or that it might end being inconsistent with keys in the future which is to become a subset matcher in v1. What do you think?

The issue (#6) which @koresar raised talks about keys being assumed to be a subset matcher and I've got a feeling it will apply here, too. I'm also thinking of adding values as a corollary to keys and that should probably behave with the same semantics as keys. The word members then sounds awfully lot like a synonym for values.

So, what's your stance on this? Has any better sounding word crossed your mind? It is equivalence of sorts, but naming it equate like you originally proposed, I'm worried, will still be too ambiguous.

Some names that I could think of were onlyMembers (we'll probably need an onlyKeys once keys becomes a subset matcher) and set:

[1, 2, 3].must.have.onlyMembers([1, 2])
[1, 2, 3].must.be.set([1, 2])

The latter could, in theory, interfere with the typeof matchers like foo.must.be.a.date when ECMAScript 6 comes to play with its built-in Set.

Another could be eq following the style of the shorter the "equals" name the less strict it is. Currently equal checks for strict equality, eql checks for strict equality of values recursively within arrays and objects. But then again, having eq may hint that it's supposed to check for strict equality recursively while also ignoring orderings of arrays. The members you proposed checks just for strict equality without recursion...

Anyways, as Phil Karlton said:

There are only two hard things in Computer Science: cache invalidation and naming things.

@moll
Copy link
Owner

moll commented May 22, 2014

A side note to remember when testing with a members-style matcher: It will match if a given array has duplicate elements. That might or might not be what you want. ;-)

[1, 2, 3, 2, 1].must.have.members([1, 2, 3])

@bajtos
Copy link
Contributor Author

bajtos commented May 22, 2014

The word members then sounds awfully lot like a synonym for values.

Agreed. Perhaps members is better at expressing the intent than values. A variable usually has a single value. When the value is an array/set/hashmap, then it can have multiple members.

Regardless of members vs. values, we need to figure out how to distinguish between a subset matcher and all-values matcher. The competitor chaijs uses chains for this, you get expect(foo).to.include.members and expect(foo).to.have.members.

@bajtos
Copy link
Contributor Author

bajtos commented May 22, 2014

Few ideas:

  • must.be.subsetOf instead of must.have.members, must.be.setEql as a shortcut for "be subset" and "be superset"
  • must.be.equivalent as a longer version of equate
  • must.be.permutationOf - this has to check that the duplicated elements are repeated in both lists.

I think permutation is the best term to use as it has very clear meaning.

@moll
Copy link
Owner

moll commented May 22, 2014

Absolutely, no problem with adding both members and values and making one an alias of the other.

Indeed, Chai has its chainable thingies. While I'd like to simplify this and have a separate dedicated matcher for separate behaviors, even with their chains they must've picked one as the default. Looks to me like they picked subset (http://chaijs.com/api/bdd/#members). Chai's assert interface has separated them to sameMembers and includeMembers (http://chaijs.com/api/assert/#sameMembers).

@moll
Copy link
Owner

moll commented May 22, 2014

must.be.subsetOf instead of must.have.members, must.be.setEql as a shortcut for "be subset" and "be superset"

You probably meant supersetOf. :-D You've got to love reasoning about sentences like "a is a superset of b which is a subset of c".

must.be.equivalent as a longer version of equate

Following the unwritten rule of longer being stricter, that wouldn't work well. :/

must.be.permutationOf - this has to check that the duplicated elements are repeated in both lists.

That's a great idea! I'm with you that it's so far the best.

@bajtos
Copy link
Contributor Author

bajtos commented May 22, 2014

Reworked to permutationOf.

Assert the array have the same members, possibly in a different order.
@bajtos bajtos mentioned this pull request May 26, 2014
@moll moll merged commit 94e2929 into moll:master May 28, 2014
@moll
Copy link
Owner

moll commented May 28, 2014

Thank you again! Merged and released v0.12.0 with Must.prototype.permutationOf.

@bajtos bajtos changed the title Add Must.prototype.members Add Must.prototype.permutationOf Jun 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants