Skip to content

doc: Update tools/icu/README.md - #16939

Closed
srl295 wants to merge 0 commit into
nodejs:masterfrom
srl295:update-icu-guide
Closed

doc: Update tools/icu/README.md#16939
srl295 wants to merge 0 commit into
nodejs:masterfrom
srl295:update-icu-guide

Conversation

@srl295

@srl295srl295 commented Nov 10, 2017•
edited
Loading

Copy link
Copy Markdown
Member
  • remove TODOs: the one about defaults has been
    addressed, and the one about testing is a work
    item that doesn't belong in a doc.
  • add some background information

Fixes: #7843

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@srl295srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Nov 10, 2017
@srl295srl295 self-assigned this Nov 10, 2017
@nodejs-github-botnodejs-github-bot added i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Nov 10, 2017
mathiasbynens
mathiasbynens reviewed Nov 12, 2017
View reviewed changes
Comment threadtools/icu/README.md Outdated

@mathiasbynensmathiasbynensNov 12, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: not just Intl.* — it‘s also used for e.g. RegExp Unicode property escapes.

@addaleaxaddaleaxNov 18, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mathiasbynens Would just using internationalization functionality instead of Intl functionality sound good to you?

@mathiasbynensmathiasbynensNov 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that sounds good!

@srl295srl295Jan 30, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

(dramatic pause) 👍

mhdawson
mhdawson approved these changes Nov 16, 2017
View reviewed changes

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@apapirovski

apapirovski commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

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

I would like to land this soon so will implement the change suggested by @mathiasbynens myself. It's been sitting around for almost a month now due to that tiny little thing.

@BridgeAR

BridgeAR commented Jan 19, 2018

Copy link
Copy Markdown
Member

New Mini-CI (seemed like the old one failed?): https://ci.nodejs.org/job/node-test-commit-light/148/

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2018
srl295 added a commit that referenced this pull request Jan 30, 2018
@srl295
doc: Update tools/icu/README.md
07712d1
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: #7843
PR-URL: #16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@srl295srl295 closed this Jan 30, 2018
@srl295
srl295 deleted the update-icu-guide branch January 30, 2018 00:35
@apapirovskiapapirovski mentioned this pull request Jan 30, 2018
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@srl295@MylesBorins
doc: Update tools/icu/README.md
f86f6a7
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: #7843
PR-URL: #16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@srl295@MylesBorins
doc: Update tools/icu/README.md
8ce2368
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: #7843
PR-URL: #16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@srl295@MylesBorins
doc: Update tools/icu/README.md
2d968ca
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: #7843
PR-URL: #16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

Does this need to be backported?

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@srl295@MayaLekova
doc: Update tools/icu/README.md
92c6b9c
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: nodejs#7843
PR-URL: nodejs#16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@srl295@MylesBorins
doc: Update tools/icu/README.md
1af6ac2
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: #7843
PR-URL: #16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@srl295@MylesBorins
doc: Update tools/icu/README.md
21de715
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: #7843
PR-URL: #16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorinsMylesBorins mentioned this pull request Jul 9, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@srl295@rvagg
doc: Update tools/icu/README.md
fceeee6
- remove TODOs: the one about defaults has been
addressed, and the one about testing is a work
item that doesn't belong in a doc.
- add some background information
Fixes: #7843
PR-URL: #16939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This was referenced Mar 9, 2020
Open
Open
@snyk-botsnyk-bot mentioned this pull request Jul 15, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax left review comments
@jasnelljasnelljasnell approved these changes
+2 more reviewers
@mathiasbynensmathiasbynensmathiasbynens left review comments
@mhdawsonmhdawsonmhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@srl295srl295

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.i18n-apiIssues and PRs related to the i18n implementation.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

deps: fix TODO in tools/icu/README.md

9 participants

@srl295@apapirovski@BridgeAR@MylesBorins@mathiasbynens@jasnell@addaleax@mhdawson@nodejs-github-bot