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

Integrated chai-subset and added assert-based negation to containSubset #1664

Merged
merged 3 commits into from
Jan 23, 2025

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
test/subset.js Outdated Show resolved Hide resolved
lib/chai/core/assertions.js Outdated Show resolved Hide resolved
lib/chai/core/assertions.js Outdated Show resolved Hide resolved
@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

lib/chai/core/assertions.js Outdated Show resolved Hide resolved
lib/chai/core/assertions.js Outdated Show resolved Hide resolved
@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