Skip to content

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

Closed
BridgeAR wants to merge 5 commits intonodejs:masterfrom
BridgeAR:align-deep-equal-nan
Closed

assert: handle (deep) equal(NaN, NaN) as being identical#30766
BridgeAR wants to merge 5 commits intonodejs:masterfrom
BridgeAR:align-deep-equal-nan

Conversation

@BridgeAR
Copy link
Copy Markdown
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.

Comment thread test/parallel/test-assert.js
@nodejs-github-bot
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

nodejs-github-bot commented Dec 6, 2019

@BridgeAR
Copy link
Copy Markdown
Member Author

BridgeAR commented Dec 6, 2019

@Trott I updated the documentation accordingly.

Comment thread doc/api/assert.md Outdated
Comment thread doc/api/assert.md Outdated
Comment thread doc/api/assert.md Outdated
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Dec 6, 2019

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 <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR
Copy link
Copy Markdown
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