src, doc: improve documentation and error message for ICU data fallback by joyeecheung · Pull Request #49666 · nodejs/node · GitHub

src: improve error message when ICU data cannot be initialized

Previously when we fail to initialize ICU data, the error message is

could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir
parameters)

This patch updates it to something similar to:

U_FILE_ACCESS_ERROR: Could not initialize ICU. Check the directory
specified by NODE_ICU_DATA or --icu-data-dir contains icudt73l.dat
and it's readable

Where the expected data file name is the same as U_ICUDATA_NAME.

doc: improve documentation about ICU data fallback

This patch:

Documents --with-icu-default-data-dir and its precedence
Elaborates a bit more about the format of the name of the expected
data file.

Review requested:

@nodejs/startup

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

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

Just curious, why std::string* and not const std::string&?

This is an out parameter, the caller gets the error string if it fails

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

Fixed the line length to make the linter happy

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

Previously when we fail to initialize ICU data, the error message is ``` could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters) ``` This patch updates it to something similar to: ``` U_FILE_ACCESS_ERROR: Could not initialize ICU. Check the directory specified by NODE_ICU_DATA or --icu-data-dir contains icudt73l.dat and it's readable ``` Where the expected data file name is the same as U_ICUDATA_NAME.

This patch: - Documents `--with-icu-default-data-dir` and its precedence - Elaborates a bit more about the format of the name of the expected data file.

Apparently test-icu-data-dir was matching the old error messages. Updated the test a bit.

@richardlau @jasnell I've updated the test for new error message matches. Can you take a look again? Thanks

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

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

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

Landed in c2cd744...db5e993

Previously when we fail to initialize ICU data, the error message is ``` could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters) ``` This patch updates it to something similar to: ``` U_FILE_ACCESS_ERROR: Could not initialize ICU. Check the directory specified by NODE_ICU_DATA or --icu-data-dir contains icudt73l.dat and it's readable ``` Where the expected data file name is the same as U_ICUDATA_NAME. PR-URL: #49666 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This patch: - Documents `--with-icu-default-data-dir` and its precedence - Elaborates a bit more about the format of the name of the expected data file. PR-URL: #49666 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Previously when we fail to initialize ICU data, the error message is ``` could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters) ``` This patch updates it to something similar to: ``` U_FILE_ACCESS_ERROR: Could not initialize ICU. Check the directory specified by NODE_ICU_DATA or --icu-data-dir contains icudt73l.dat and it's readable ``` Where the expected data file name is the same as U_ICUDATA_NAME. PR-URL: #49666 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This patch: - Documents `--with-icu-default-data-dir` and its precedence - Elaborates a bit more about the format of the name of the expected data file. PR-URL: #49666 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 15, 2023

richardlau approved these changes Sep 15, 2023

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2023

github-actions Bot mentioned this pull request Sep 16, 2023

CI Reliability 2023-09-16 nodejs/reliability#667

Open

25 tasks

jasnell reviewed Sep 16, 2023

View reviewed changes

jasnell approved these changes Sep 16, 2023

View reviewed changes

github-actions Bot mentioned this pull request Sep 17, 2023

CI Reliability 2023-09-17 nodejs/reliability#668

Open

32 tasks

github-actions Bot mentioned this pull request Sep 18, 2023

CI Reliability 2023-09-18 nodejs/reliability#669

Open

33 tasks

joyeecheung force-pushed the small-icu-doc branch from 81991de to ac4edac Compare September 18, 2023 07:29

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023

joyeecheung added 2 commits September 18, 2023 12:32

doc: improve documentation about ICU data fallback

930aba9

This patch: - Documents `--with-icu-default-data-dir` and its precedence - Elaborates a bit more about the format of the name of the expected data file.

joyeecheung force-pushed the small-icu-doc branch from ac4edac to 930aba9 Compare September 18, 2023 10:32

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023

richardlau approved these changes Sep 18, 2023

View reviewed changes

This was referenced Sep 19, 2023

CI Reliability 2023-09-19 nodejs/reliability#670

Open

CI Reliability 2023-09-20 nodejs/reliability#671

Open

CI Reliability 2023-09-21 nodejs/reliability#672

Open

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Sep 22, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023

nodejs-github-bot closed this Sep 22, 2023

This was referenced Sep 28, 2023

v20.7.1 proposal #49917

Closed

v20.8.0 proposal #49932

Merged

src, doc: improve documentation and error message for ICU data fallback by joyeecheung · Pull Request #49666 · nodejs/node · GitHub

Uh oh!

Uh oh!

src, doc: improve documentation and error message for ICU data fallback - #49666

src, doc: improve documentation and error message for ICU data fallback#49666joyeecheung wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom joyeecheung:small-icu-docjoyeecheung/node:small-icu-docCopy head branch name to clipboard

Conversation

joyeecheung commented Sep 15, 2023

src: improve error message when ICU data cannot be initialized

doc: improve documentation about ICU data fallback

Uh oh!

nodejs-github-bot commented Sep 15, 2023

Uh oh!

nodejs-github-bot commented Sep 15, 2023

Uh oh!

nodejs-github-bot commented Sep 16, 2023

Uh oh!

jasnell Sep 16, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheung Sep 16, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 17, 2023

Uh oh!

joyeecheung commented Sep 18, 2023

Uh oh!

nodejs-github-bot commented Sep 18, 2023

Uh oh!

joyeecheung commented Sep 18, 2023

Uh oh!

joyeecheung commented Sep 18, 2023

Uh oh!

nodejs-github-bot commented Sep 18, 2023

Uh oh!

nodejs-github-bot commented Sep 18, 2023

Uh oh!

nodejs-github-bot commented Sep 18, 2023

Uh oh!

nodejs-github-bot commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

src, doc: improve documentation and error message for ICU data fallback#49666
joyeecheung wants to merge 2 commits into
nodejs:mainfrom
joyeecheung:small-icu-doc