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.

Sign up for GitHub

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

Jump to bottom

util: add null prototype support for date #25144

Closed
wants to merge 1 commit into from
Closed

util: add null prototype support for date #25144

wants to merge 1 commit into from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Dec 20, 2018 •
edited
Loading

Adds the support of null prototype in Date object.

cc @BridgeAR

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

@nodejs-github-bot
Copy link
Collaborator

@antsmartian build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2014/pipeline

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 20, 2018
@antsmartian antsmartian mentioned this pull request Dec 20, 2018
Closed
BridgeAR
BridgeAR reviewed Dec 20, 2018
View reviewed changes
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.

Since invalid dates will now be handled properly with keys as well, please also add a test for that (as well as subclassing if implemented).

lib/internal/util/inspect.js Outdated Show resolved Hide resolved
test/parallel/test-util-inspect.js Outdated Show resolved Hide resolved
@antsmartian antsmartian force-pushed the date_util branch 2 times, most recently from 43c8edf to 435415f Compare December 21, 2018 02:43
@antsmartian
Copy link
Contributor Author

@mscdex @BridgeAR PTAL...

@antsmartian
Copy link
Contributor Author

antsmartian commented Dec 21, 2018 •
edited
Loading

CI : https://ci.nodejs.org/job/node-test-pull-request/19709/
CI : https://ci.nodejs.org/job/node-test-pull-request/19711/

@antsmartian antsmartian force-pushed the date_util branch 2 times, most recently from 2a9c063 to 5f8d3a9 Compare December 21, 2018 11:06
BridgeAR
BridgeAR reviewed Dec 21, 2018
View reviewed changes
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.

This is LG to me if the keys.length === 0 check is removed (the original conditional was a mistake in the first place and with the current implementation it produces a weird output).

lib/internal/util/inspect.js Outdated Show resolved Hide resolved
@antsmartian antsmartian force-pushed the date_util branch 2 times, most recently from 840315d to 58b2aba Compare December 21, 2018 12:33
BridgeAR
BridgeAR approved these changes Dec 21, 2018
View reviewed changes
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.

LGTM

@antsmartian
Copy link
Contributor Author

antsmartian commented Dec 21, 2018 •
edited
Loading

Thanks for guiding me here @BridgeAR. I love utils, may be few more PR's on the way!

I guess we are now left with RE and Errors null proto handling. Will try out working on RE and send out a PR in a few days.

@antsmartian
Copy link
Contributor Author

CI : https://ci.nodejs.org/job/node-test-pull-request/19714/

@jdalton
Copy link
Member

jdalton commented Dec 21, 2018

I've had this bite me for other non-date values. Might be a good time to review other places this can happen.

@antsmartian
Copy link
Contributor Author

@jdalton Yes. We have already support for other types apart from Error and RE.

@antsmartian
Copy link
Contributor Author

antsmartian commented Dec 22, 2018 •
edited by Trott
Loading

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/19748/ ✔️

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

@nodejs/util this could use another LG.

@antsmartian this needs a rebase.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 31, 2018
@antsmartian
Copy link
Contributor Author

New CI After Rebase: https://ci.nodejs.org/job/node-test-pull-request/19966/

@antsmartian
Copy link
Contributor Author

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/19986/

@antsmartian
Copy link
Contributor Author

Fresh CI (after rebase due to conflict): https://ci.nodejs.org/job/node-test-pull-request/20009/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 9, 2019
@antsmartian
Copy link
Contributor Author

@BridgeAR @jasnell Rebased the codebase, do you want to have a look once before I land this? I'm planning to land it soon.

@antsmartian
Copy link
Contributor Author

CI : https://ci.nodejs.org/job/node-test-pull-request/20020/

@BridgeAR
Copy link
Member

Landed in 81b25ea 🎉

@BridgeAR BridgeAR closed this Jan 10, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 10, 2019
@antsmartian @BridgeAR
util: add null prototype support for date
81b25ea 
PR-URL: nodejs#25144
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@antsmartian antsmartian deleted the date_util branch January 10, 2019 02:49
addaleax pushed a commit that referenced this pull request Jan 14, 2019
@antsmartian @addaleax
util: add null prototype support for date
2e6e4cf 
PR-URL: #25144
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@antsmartian @BridgeAR
util: add null prototype support for date
7dab908 
PR-URL: nodejs#25144
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@mscdex mscdex mscdex left review comments

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@antsmartian @nodejs-github-bot @jdalton @BridgeAR @mscdex @jasnell @Trott @addaleax