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

util: extract uncurryThis function for reuse #23081

Closed
wants to merge 2 commits into from

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
lib/internal/util/comparisons.js Outdated Show resolved Hide resolved
test/parallel/test-bootstrap-modules.js Outdated Show resolved Hide resolved
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

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

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

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 ZYSzys force-pushed the extract-util branch 2 times, most recently from 03a1c48 to f66230b Compare October 9, 2018 02:09
@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

@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

@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

@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

@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

Landed in 20fab5f, 5f032a7 🎉

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 21, 2019
PR-URL: nodejs#23081
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 21, 2019
PR-URL: nodejs#23081
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Mar 28, 2019
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Mar 30, 2019
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Mar 30, 2019
PR-URL: #23081
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.