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

assert: handle (deep) equal(NaN, NaN) as being identical #30766

Closed
wants to merge 5 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 2, 2019

This aligns the equal and deepEqual() implementation with the
strict version by accepting NaN as being identical in
case both sides are NaN.

Refs: #30350 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This aligns the `equal` and `deepEqual()` implementations with the
strict versions by accepting `NaN` as being identical in case both
sides are NaN.

Refs: nodejs#30350 (comment)
@BridgeAR BridgeAR requested a review from addaleax December 2, 2019 17:15
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. util Issues and PRs related to the built-in util module. labels Dec 2, 2019
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 2, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

test/parallel/test-assert.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 3, 2019
@Trott
Copy link
Member

Trott commented Dec 5, 2019

This probably requires an update to assert.md since this diverges from the Abstract Equality Comparison cited in the doc?

@Trott
Copy link
Member

Trott commented Dec 5, 2019

This probably requires an update to assert.md since this diverges from the Abstract Equality Comparison cited in the doc?

I mean, I see the YAML is updated, but I think the actual text for assert.equal() in Legacy mode needs to change? And the details for assert.deepEqual() probably need an update too?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 6, 2019

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

@Trott I updated the documentation accordingly.

doc/api/assert.md Outdated Show resolved Hide resolved
doc/api/assert.md Outdated Show resolved Hide resolved
doc/api/assert.md Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 6, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27419/ ✅ (yellow build with two Windows flakes)

BridgeAR added a commit that referenced this pull request Dec 6, 2019
This aligns the `equal` and `deepEqual()` implementations with the
strict versions by accepting `NaN` as being identical in case both
sides are NaN.

Refs: #30350 (comment)

PR-URL: #30766
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

Landed in 5360dd1 🎉

@BridgeAR BridgeAR closed this Dec 6, 2019
@BridgeAR BridgeAR deleted the align-deep-equal-nan branch January 20, 2020 12:07
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants