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

Implement reference comparison #218

Merged
merged 5 commits into from
Apr 21, 2019
Merged

Conversation

jakubzitny
Copy link
Contributor

This MR adds referenceEqual BDD method and referenceEqual and referenceNotEqual TDD methods according to the discussion in #210.

Close #210.

@jakubzitny
Copy link
Contributor Author

I am not sure how you generate the README examples, should I just manually copy the comments from the implementation to README.md @astorije?

@jankuca
Copy link

jankuca commented Apr 14, 2019

@jakubzitny Will expect().to.not.referenceEqual() work?

@jakubzitny
Copy link
Contributor Author

Yes, it's already in the test cases: https://github.com/astorije/chai-immutable/pull/218/files#diff-c1129c8b045390789fa8ff62f2c6b4a9. The to.nots and not.tos and similar chaining calls are handled by chai out of the box in the BDD API.

@astorije
Copy link
Owner

I am not sure how you generate the README examples, should I just manually copy the comments from the implementation to README.md @astorije?

Yup, it's a very complex build step that involves copying and pasting 😅
Thanks for opening this, @jakubzitny, that's awesome, I'll be looking into your PR very shortly (and if I don't, it's because I dropped the ball and feel free to ping me!)

@astorije astorije added this to the next milestone Apr 15, 2019
@astorije
Copy link
Owner

@jakubzitny, I took the liberty to add the doc in the README, and also renaming referenceNotEqual into notReferenceEqual and editing the examples a little to make them pass tests.
How do you feel about those changes? It's pretty late here so I'll let you double-check me before merging 😅

Copy link
Owner

@astorije astorije left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your help!! 🙏

@jakubzitny
Copy link
Contributor Author

Changes are OK. The commit messages could be better, but that's your call 😉 I can squash the notReferenceEqual to my implementation commit if you want. Or we can just merge it as it is, again your call 🙂

@astorije
Copy link
Owner

Heh, that's fine for this one.
Thank you very much for your help!

@astorije astorije merged commit 93b70a7 into astorije:master Apr 21, 2019
@astorije
Copy link
Owner

This has now be released as part of v2.1.0. Thanks again!

astorije added a commit that referenced this pull request Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support reference comparison
3 participants