Skip to content

Conversation

@BreadInvasion
Copy link
Contributor

A revival of @koddsson 's previous attempt at merging the abandoned chai-subset functionality, with some added comments and an assert-style negation of containSubset which was not present in the original package. I know there was some concern about code duplication in the original PR, so I'm happy to respond to feedback! Thanks all.

Fixes #1616

@BreadInvasion BreadInvasion requested a review from a team as a code owner January 17, 2025 20:22
@43081j
Copy link
Contributor

43081j commented Jan 18, 2025

we should also add tests for using this with should and assert style assertions

otherwise looks good so far though 👍

@BreadInvasion
Copy link
Contributor Author

@43081j I'm not sure if it's necessary to run the full battery of tests in each assertion style - is it not enough to ensure that they actually call the right function, since all three styles ultimately just call the same containsSubset function? We do this at 147-168 of the test file.

@BreadInvasion
Copy link
Contributor Author

Fixed merge conflict with the change in method of declaring aliases. Ready for re-review.

koddsson
koddsson previously approved these changes Jan 22, 2025
Copy link
Member

@koddsson koddsson left a comment

Choose a reason for hiding this comment

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

This is looking good to me but I'll let @43081j have the final approval

@43081j
Copy link
Contributor

43081j commented Jan 23, 2025

looks good to me 👍

big thanks for implementing this 🙏

@43081j 43081j merged commit da2e109 into chaijs:main Jan 23, 2025
5 checks passed
@koddsson
Copy link
Member

Thanks @BreadInvasion ❤️

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

3 participants