Skip to content

src: remove icuDataDir from node config#24780

Closed
GauthamBanasandra wants to merge 1 commit into
nodejs:masterfrom
GauthamBanasandra:node-cfg
Closed

src: remove icuDataDir from node config#24780
GauthamBanasandra wants to merge 1 commit into
nodejs:masterfrom
GauthamBanasandra:node-cfg

Conversation

@GauthamBanasandra
Copy link
Copy Markdown
Contributor

@GauthamBanasandra GauthamBanasandra commented Dec 2, 2018

icuDataDir seems to be redundant as it is not used anywhere.
Hence removing it.

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

@GauthamBanasandra
src: remove icuDataDir from node config
2fb2700 
icuDataDir seems to be redundant as it is not used anywhere.
Hence removing it.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 2, 2018
addaleax
addaleax approved these changes Dec 2, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

/cc @srl295

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 2, 2018
@addaleax
Copy link
Copy Markdown
Member

addaleax commented Dec 2, 2018

Labelling semver-major out of caution.

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

@GauthamBanasandra
Copy link
Copy Markdown
Contributor Author

GauthamBanasandra commented Dec 2, 2018

I see that 2 tests have failed. May I know how I can replicate this locally?

@addaleax
Copy link
Copy Markdown
Member

addaleax commented Dec 4, 2018 •
edited by Trott
Loading

@GauthamBanasandra These have been flaky for a while, I think…

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

(Either way – I’ve labelled this semver-major out of caution, even though this is only internal code, so another @nodejs/tsc review would be necessary.)

@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Dec 4, 2018

Anyone from @nodejs/intl knows about the usage of this?

@srl295
Copy link
Copy Markdown
Member

srl295 commented Dec 4, 2018

looking

srl295
srl295 approved these changes Dec 4, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

I can confirm this is not used. It will have the value of the --icu-data-dir parameter or else of NODE_ICU_DATA. I thought it had a reason when I put it in, but I think it is fine to remove. It's definitely not API.

@srl295
Copy link
Copy Markdown
Member

srl295 commented Dec 4, 2018

hold on… doing some more digging… 

@srl295
Copy link
Copy Markdown
Member

srl295 commented Dec 4, 2018

This variable was used to determine whether to disable v8BreakIterator.

The use case was removed in https://github.com/nodejs/node/pull/15238/files#diff-5718e956bb67bf0cf12a046fde716343L141 by a better fix ( PR #15238 ), so this probably was simply missed there. cc @bnoordhuis

Other PRs that modified this were #11255 and #14868

@joyeecheung joyeecheung mentioned this pull request Dec 5, 2018
Closed
@GauthamBanasandra
Copy link
Copy Markdown
Contributor Author

GauthamBanasandra commented Dec 13, 2018

People, is there anything that is pending to be done for this patch to be merged?

@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Dec 13, 2018

@GauthamBanasandra This is semver-major so needs another TSC approval

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

Trott commented Dec 14, 2018

Since it's semver-major, the last step is a CITGM run.

@Trott
Copy link
Copy Markdown
Member

Trott commented Dec 14, 2018

Hopefully I did this correctly... CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1687/

@Trott
Copy link
Copy Markdown
Member

Trott commented Dec 15, 2018

@nodejs/citgm Do CITGM results for this look OK?

@targos
Copy link
Copy Markdown
Member

targos commented Dec 15, 2018

CITGM results LGTM

@Trott
Copy link
Copy Markdown
Member

Trott commented Dec 15, 2018

Landed in 9021b0d

@Trott Trott closed this Dec 15, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 15, 2018
@GauthamBanasandra @Trott
src: remove icuDataDir from node config
9021b0d 
icuDataDir seems to be redundant as it is not used anywhere.
Hence removing it.

PR-URL: nodejs#24780
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@GauthamBanasandra @refack
src: remove icuDataDir from node config
07b6342 
icuDataDir seems to be redundant as it is not used anywhere.
Hence removing it.

PR-URL: nodejs#24780
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Mar 26, 2019
Merged
BethGriggs added a commit that referenced this pull request Apr 22, 2019
@BethGriggs