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

doc: add documentation on ICU #13916

Closed
wants to merge 6 commits into from
Closed

Conversation

TimothyGu
Copy link
Member

Adapted from the wiki article "Intl" by @srl295. Open to suggestions w.r.t. where this page belongs best, either here or on nodejs.org, but somewhere more permanent than a wiki.

Refs: #13644 (comment)

Checklist
Affected core subsystem(s)

doc

@TimothyGu TimothyGu requested a review from srl295 June 26, 2017 05:12
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 26, 2017
@TimothyGu TimothyGu added the i18n-api Issues and PRs related to the i18n implementation. label Jun 26, 2017
@vsemozhetbyt
Copy link
Contributor

If it remains in the api, it should also be added to all.md

@vsemozhetbyt
Copy link
Contributor

doc/api/intl.md Outdated
in native C/C++ code. However, some of them require a very large ICU data file
in order to support all locales of the world. Since most Node.js users will
make use of only a small section in the full ICU data set, we provide several
options for customizing ICU support in a Node.js build.
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid use of informal pronouns like we in the docs... Also, this last sentence is a bit off for me... Perhaps:

Because it is expected that most Node.js users will make use of only a small portion
of ICU functionality, only a subset of the full ICU data set is provided by Node.js by
default. Several options are provided for customizing and expanding the ICU data set
either when building or running Node.js.

doc/api/intl.md Outdated

Node.js can link against an ICU build already installed on the system. In fact,
most Linux distributions already come with ICU installed, and this option would
make it possible to reuse the same set of data used by other components in your
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid using informal pronouns like your in the docs.

doc/api/intl.md Outdated

#### Providing ICU data at runtime

If you use the `small-icu` option, you can still provide additional locale data
Copy link
Member

Choose a reason for hiding this comment

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

As above, please avoid using informal pronouns like you

@TimothyGu
Copy link
Member Author

@jasnell Comments are addressed. What are your thoughts on where this document should end up?

@TimothyGu
Copy link
Member Author

I'll merge this tomorrow if there are no more comments.

@vsemozhetbyt
Copy link
Contributor

Linter (you may need to rebase after the 15599cb):

  96:1  error  Expected indentation of 0 spaces but found 2  indent
  98:1  error  Expected indentation of 0 spaces but found 2  indent
  99:1  error  Expected indentation of 0 spaces but found 2  indent

@TimothyGu
Copy link
Member Author

TimothyGu commented Jul 5, 2017

Not too big of a fan of the linter change, but changed.

Linter CI: https://ci.nodejs.org/job/node-test-linter/10274/

@vsemozhetbyt
Copy link
Contributor

@TimothyGu It seems it is due to 7477f9b, but I wanted to be on safe side with updated tools)

@TimothyGu
Copy link
Member Author

Landed in 6809429

@TimothyGu TimothyGu closed this Jul 6, 2017
@TimothyGu TimothyGu deleted the intl-doc branch July 6, 2017 03:21
TimothyGu added a commit that referenced this pull request Jul 6, 2017
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@addaleax addaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@MylesBorins
Copy link
Contributor

Is this applicable to v6.x? It is easy enough to land this but I want to ensure that it makes sense for the version of ICU that is embedded

6.x: 58.2
8.x: 59.1

@TimothyGu
Copy link
Member Author

@MylesBorins Yep, most of the content is applicable to 6.x as well, but some things need to be changed (e.g. mentions of the WHATWG URL API as it doesn't exist in 6.x).

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

post merge 👍

TimothyGu added a commit to TimothyGu/node that referenced this pull request Sep 15, 2017
PR-URL: nodejs#13916
Refs: nodejs#13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants