Skip to content

util: extract uncurryThis function for reuse#23081

Closed
ZYSzys wants to merge 2 commits intonodejs:masterfrom
zys-contrib:extract-util
Closed

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

Conversation

@ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Sep 25, 2018

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 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
jasnell reviewed Sep 25, 2018
View reviewed changes
BridgeAR
BridgeAR approved these changes Sep 25, 2018
View reviewed changes
Fishrock123
Fishrock123 previously requested changes Sep 26, 2018
View reviewed changes
lundibundi
lundibundi approved these changes Sep 26, 2018
View reviewed changes
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

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

@ZYSzys
Copy link
Member Author

ZYSzys commented Sep 26, 2018 •
edited
Loading

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
Copy link
Contributor

refack commented Sep 27, 2018

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
Copy link
Contributor

thefourtheye commented Oct 6, 2018

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

@refack
Copy link
Contributor

refack commented Oct 6, 2018

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
Copy link
Member

lundibundi commented Oct 8, 2018

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
Copy link
Member Author

ZYSzys commented Oct 9, 2018

It was changed now.
But I'm a little confused about the linter error.
screen shot 2018-10-09 at 10 16 38 am

How should I fixed it ?

@lundibundi
Copy link
Member

lundibundi commented Oct 9, 2018 •
edited
Loading

@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
Copy link
Member Author

ZYSzys commented Oct 24, 2018

@Fishrock123 @lundibundi PTAL

@refack refack mentioned this pull request Nov 6, 2018
Closed
@ZYSzys
Copy link
Member Author

ZYSzys commented Dec 20, 2018

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
Copy link
Member

lundibundi commented Dec 20, 2018

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

@ZYSzys ZYSzys changed the title lib: extract uncurryThis function util: extract uncurryThis function for reuse Feb 10, 2019
@ZYSzys
Copy link
Member Author

ZYSzys commented Mar 19, 2019 •
edited
Loading

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

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

BridgeAR commented Mar 21, 2019

Landed in 20fab5f, 5f032a7 🎉

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 21, 2019
@ZYSzys @BridgeAR
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
@ZYSzys @BridgeAR
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 BridgeAR closed this Mar 21, 2019
@ZYSzys ZYSzys deleted the extract-util branch March 22, 2019 01:08
targos pushed a commit that referenced this pull request Mar 28, 2019
@ZYSzys @targos
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
@ZYSzys @targos
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
@ZYSzys @targos
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
@ZYSzys @targos
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 BethGriggs mentioned this pull request Apr 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments

@addaleax addaleax addaleax approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+7 more reviewers

@refack refack refack approved these changes

@Fishrock123 Fishrock123 Fishrock123 left review comments

@devsnek devsnek devsnek left review comments

@jdalton jdalton jdalton approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@lundibundi lundibundi lundibundi approved these changes

@starkwang 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 pending requests for changes, 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.