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

Integrate chai-subset into chai core #1621

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

koddsson
Copy link
Member

I tried to make sure to preserve the chai-subset history when doing this.

Fixes #1616

eagleeye and others added 24 commits July 14, 2014 17:03
Use chai.use for adding method
v1.1.0 assert style of test, rm jshint, add jscs eslint, better coverage, fixes chaijs#13
v1.1.0 assert style of test, rm jshint, add jscs eslint, better coverage, fixes chaijs#13
If two objects are the same, then inherently they're subsets of each other. This also fixes a bug I've run into with testing recursive data structures.
…ijs#66)

* Adding compare function to be used in expected object template

* Adding compare function example to README.md
…ijs#66)

* Adding compare function to be used in expected object template

* Adding compare function example to README.md
@koddsson koddsson requested a review from a team as a code owner May 20, 2024 09:18
@43081j
Copy link
Contributor

43081j commented May 20, 2024

it looks good to me but i have a couple of thoughts:

1 - i know you put effort into keeping the git history but i'd actually squash this anyway if we merge it as i don't think the subset history makes sense in our tree. it may be better to just note somewhere this was originally built by andrii

2 - something's bugging me about the overlap here of includes. we have basically the same algorithm in includes (written differently) than we do here, but in that one we don't do partial equality on the deep properties.

i wonder if we can at least extract the traversal logic so they both share it, or we make includes have a flag that achieves what subset does. im cautious of putting too much logic in includes though as its already a pretty big, multi-use assertion.

@keithamus
Copy link
Member

2 - something's bugging me about the overlap here of includes.

We'll likely have similar logic in deep-eql too. I'll add that deep-eql is also very well optimised. I wonder if we can leverage it and it's custom comparators to effectively traverse the first layer?

@koddsson
Copy link
Member Author

I'll start looking at squaring this up according to the comments.

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.

chai-subset is unmaitained
7 participants