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

doc: WeakSet and WeakMap comparison details #56648

Conversation

ishon19
Copy link
Contributor

@ishon19 ishon19 commented Jan 17, 2025

This PR updates the documentation of the comparison details of the WeakMap and WeakSet as per the current behavior.

Fixes #56640

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Jan 17, 2025
Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 20, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 20, 2025
@nodejs-github-bot nodejs-github-bot merged commit 7bc2946 into nodejs:main Jan 20, 2025
21 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7bc2946

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
While this is already merged, a few outputs seem to be a bit off. I guess it's not critical, since the overall meaning will still be there.

Comment on lines +915 to +916
// + WeakSet { <items unknown> }
// - WeakSet { <items unknown> }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// + WeakSet { <items unknown> }
// - WeakSet { <items unknown> }
// WeakSet {
// <items unknown>
// }

weakSet2.add(obj);

// Comparing different instances fails, even with same contents
assert.deepStrictEqual(weakSet1, weakSet2);
// AssertionError: Expected inputs to be strictly deep-equal:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// AssertionError: Expected inputs to be strictly deep-equal:
// AssertionError: Values have same structure but are not reference-equal:

weakSet2.add(obj);

// Comparing different instances fails, even with same contents
assert.deepStrictEqual(weakSet1, weakSet2);
// AssertionError: Expected inputs to be strictly deep-equal:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// AssertionError: Expected inputs to be strictly deep-equal:
// AssertionError: Values have same structure but are not reference-equal:

Comment on lines +1024 to +1025
// + WeakSet { <items unknown> }
// - WeakSet { <items unknown> }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// + WeakSet { <items unknown> }
// - WeakSet { <items unknown> }
// WeakSet {
// <items unknown>
// }

@ishon19
Copy link
Contributor Author

ishon19 commented Jan 21, 2025

Thank you for the PR! While this is already merged, a few outputs seem to be a bit off. I guess it's not critical, since the overall meaning will still be there.

Thanks for the review @BridgeAR! My bad, not sure if I forgot pushing some commit, but I can create another PR real quick addressing the requested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated comparison details for WeakSet and WeakMap
6 participants