FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: extract `uncurryThis` function for reuse by ZYSzys · Pull Request #23081 · nodejs/node · GitHub

/ node Public

util: extract uncurryThis function for reuse - #23081

Closed
ZYSzys wants to merge 2 commits into
nodejs:masterfrom
zys-contrib:extract-util
ConversationCommits (2)ChecksFiles changed
Closed

util: extract uncurryThis function for reuse#23081
ZYSzys wants to merge 2 commits into
nodejs:masterfrom
zys-contrib:extract-util

Conversation

ZYSzys commented Sep 25, 2018

Copy link
Copy Markdown
Member

Extracts uncurryThis function which is done in identical ways in a few places in lib/internal/util dir.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 25, 2018
jasnell reviewed Sep 25, 2018
View reviewed changes
Comment thread lib/internal/util/comparisons.js Outdated
BridgeAR approved these changes Sep 25, 2018
View reviewed changes
Comment thread lib/internal/util/comparisons.js Outdated
Fishrock123 previously requested changes Sep 26, 2018
View reviewed changes
Comment thread test/parallel/test-bootstrap-modules.js Outdated
lundibundi approved these changes Sep 26, 2018
View reviewed changes

lundibundi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Though I'm +1 for @Fishrock123 suggestion of moving this to util.js.

ZYSzys commented Sep 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Now I'm torn between keeping it in lib/internal/util/functional.jsor moving to lib/internal/util.js.

Hey guys, which one should I choose ? 😬

refack commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

I'm not sure internal/util.js is the best place, it will create something that looks like circular dependency, files in internal/util/ depending on a file in the parent directory. (e.g. internal/util/comparisons.js -> internal/util.js)

I'm very pro deduplicating code, so to me internal/util/types.js seems like the least worst place to put this in... But let's hear from others.

thefourtheye commented Oct 6, 2018

Copy link
Copy Markdown
Contributor

We already use functions from internal/util.js in internal/util/inspect.js. So one more small function wouldn't hurt, I guess.

refack commented Oct 6, 2018

Copy link
Copy Markdown
Contributor

We already use functions from internal/util.js in internal/util/inspect.js. So one more small function wouldn't hurt, I guess.

Sounds like a good compromise. Maybe add a TODO comment that we should re-evaluate this dependency in the future?

lundibundi commented Oct 8, 2018

Copy link
Copy Markdown
Member

It looks like we came to an agreement, @ZYSzys could you move the function to internal/util.js and rebase on master so this could proceed?
Also, a TODO comment proposed by @refack sounds like a good idea.

ZYSzys commented Oct 9, 2018

Copy link
Copy Markdown
Member Author

It was changed now.
But I'm a little confused about the linter error.

How should I fixed it ?

lundibundi commented Oct 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@ZYSzys don't worry, that's a known bug, it'll be fixed soon.

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

ZYSzys commented Oct 24, 2018

Copy link
Copy Markdown
Member Author

@Fishrock123 @lundibundi PTAL

refack mentioned this pull request Nov 6, 2018
Closed

ZYSzys commented Dec 20, 2018

Copy link
Copy Markdown
Member Author

ping @Fishrock123

Sorry for bothering you. It has almost been three months, and I've moved the uncurryThis function into lib/internal/util.js for reusable, can you please take a look at this PR again ?

lundibundi commented Dec 20, 2018

Copy link
Copy Markdown
Member

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

ZYSzys changed the title lib: extract uncurryThis function util: extract uncurryThis function for reuse Feb 10, 2019

ZYSzys commented Mar 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

ZYSzys added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 20, 2019

BridgeAR commented Mar 21, 2019

Copy link
Copy Markdown
Member

Landed in 20fab5f, 5f032a7 🎉

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 21, 2019
util: require isNativeError from internalBinding
20fab5f 
PR-URL: nodejs#23081
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 21, 2019
util: extract uncurryThis function for reuse
5f032a7 
PR-URL: nodejs#23081
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR closed this Mar 21, 2019
ZYSzys deleted the extract-util branch March 22, 2019 01:08
targos pushed a commit that referenced this pull request Mar 28, 2019
util: require isNativeError from internalBinding
a9914f3 
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Mar 28, 2019
util: extract uncurryThis function for reuse
bc5d89d 
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Mar 30, 2019
util: require isNativeError from internalBinding
169f3f7 
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Mar 30, 2019
util: extract uncurryThis function for reuse
21486e5 
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs mentioned this pull request Apr 9, 2019
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
addaleax addaleax approved these changes
joyeecheung joyeecheung approved these changes
BridgeAR BridgeAR approved these changes
+7 more reviewers
refack refack approved these changes
Fishrock123 Fishrock123 left review comments
devsnek devsnek left review comments
jdalton jdalton approved these changes
thefourtheye thefourtheye approved these changes
lundibundi lundibundi approved these changes
starkwang starkwang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL