src: add process.versions.icu#3102

evanlucas · 2015-09-28T12:04:19Z

If i18n support is present, add the icu version
to process.versions

Fixes: #3089

R= @silverwind / @srl295 ?

bnoordhuis · 2015-09-28T12:30:55Z

src/node.cc

I think this should be unicode/uversion.h?

Ah good call

Actually it should be uvernum.h and not uversion.h to get only the #define and not additional functions.

evanlucas · 2015-09-28T12:53:01Z

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

evanlucas · 2015-09-28T12:59:30Z

Ah, linter wasn't happy. Fixed

evanlucas · 2015-09-28T13:21:45Z

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

srl295 · 2015-09-28T14:08:07Z

.. But otherwise +1 and lgtm- I actually wondered about this before.

There are other versions here: Unicode, time zone, cldr data. Only Unicode
version is a constant, the other 2 are dynamic (I.e: load everything).

orangemocha · 2015-09-28T14:23:36Z

The last CI run had some issues (nodejs/build#204) on ARM unrelated to this change.

New CI run: https://ci.nodejs.org/job/node-test-pull-request/394/

evanlucas · 2015-09-28T14:50:38Z

Updated to use unicode/uvernum.h

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

Fishrock123 · 2015-09-28T16:35:14Z

SGTM

silverwind · 2015-09-28T17:35:42Z

Looks to be working great, thanks. Could you also update the example in the docs?

evanlucas · 2015-09-28T17:39:57Z

Updated docs as well

silverwind · 2015-09-28T17:45:41Z

doc/api/process.markdown

Not sure that comment is necessary. Strictly speaking, it'd also have to be there for openssl. I'd remove it, given that the default is to build with ICU now.

"default" is slightly misleading here. Just calling ./configure won't install ICU, but the [default] release builds you download and use will.

Yeah, of course I mean the user-facing 'default' ;)

i agree re openssl, just removing the comment will probably be the best scenario.

silverwind · 2015-09-28T18:49:32Z

LGTM

srl295 · 2015-09-28T21:17:05Z

LGTM here also

evanlucas · 2015-09-29T01:16:51Z

Awesome. Will land tomorrow if there are no objections :]

rvagg · 2015-09-29T12:53:52Z

lgtm

rvagg · 2015-09-29T12:54:18Z

tagged as semver-minor, any objections?

evanlucas · 2015-09-29T13:10:03Z

works for me :]

If i18n support is present, add the icu version to process.versions Fixes: nodejs#3089 PR-URL: nodejs#3102 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

evanlucas · 2015-09-29T13:26:20Z

Thanks! Landed in 30b8bb0

silverwind · 2015-09-29T15:25:20Z

By the way: How do you mark your manual commits as merged on Github?

evanlucas · 2015-09-29T15:27:59Z

The branch for this one was addicu

$ git remote -v mine [email protected]:evanlucas/io.js (fetch) mine [email protected]:evanlucas/io.js (push) origin [email protected]:nodejs/io.js (fetch) origin [email protected]:nodejs/io.js (push) $ git push mine +HEAD:addicu && git push origin master && git push mine :addicu

silverwind · 2015-09-29T15:30:27Z

I'll try that next time, thanks. Won't work for PRs where others are the author, I assume?

evanlucas · 2015-09-29T15:31:35Z

I don't believe so. I haven't been able to anyways

jbergstroem · 2015-09-29T19:39:47Z

@silverwind if you force-push the (final) commit to the branch a PR is tracking GH will mark the branch as merged when you fast-forward and push to master.

If i18n support is present, add the icu version to process.versions Fixes: #3089 PR-URL: #3102 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

The first Node.js LTS release! See https://github.com/nodejs/LTS/ for details of the LTS process. * **icu**: Updated to version 56 with significant performance improvements (Steven R. Loomis) #3281 * **node**: - Added new `-c` (or `--check`) command line argument for checking script syntax without executing the code (Dave Eddy) #2411 - Added `process.versions.icu` to hold the current ICU library version (Evan Lucas) #3102 - Added `process.release.lts` to hold the current LTS codename when the binary is from an active LTS release line (Rod Vagg) #3212 * **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes: https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299 PR-URL: #3258

MylesBorins · 2015-11-17T00:23:10Z

landed in v4.x-staging in 7271cb0

evanlucas added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Sep 28, 2015

bnoordhuis reviewed Sep 28, 2015
View reviewed changes

evanlucas force-pushed the addicu branch from 02283e2 to d60d21bCompare September 28, 2015 12:52

evanlucas force-pushed the addicu branch from d60d21b to 7914c7fCompare September 28, 2015 12:59

evanlucas force-pushed the addicu branch from 7914c7f to 390ffd8Compare September 28, 2015 14:47

evanlucas force-pushed the addicu branch from 390ffd8 to 984f916Compare September 28, 2015 17:39

silverwind reviewed Sep 28, 2015
View reviewed changes

evanlucas force-pushed the addicu branch from 984f916 to b234cc2Compare September 28, 2015 18:44

rvagg added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 29, 2015

src: add process.versions.icu
30b8bb0
If i18n support is present, add the icu version to process.versions Fixes: nodejs#3089 PR-URL: nodejs#3102 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

evanlucas force-pushed the addicu branch from b234cc2 to 30b8bb0Compare September 29, 2015 13:25

evanlucas closed this Sep 29, 2015

evanlucas deleted the addicu branch September 29, 2015 13:25

evanlucas merged commit 30b8bb0 into nodejs:masterSep 29, 2015

rvagg mentioned this pull request Sep 30, 2015
Release proposal: v4.1.2 #3128
Closed

jasnell added the land-on-v4.x label Oct 8, 2015

jasnell mentioned this pull request Oct 8, 2015
Preparing v4.x for LTS #3252
Closed
29 tasks

jasnell added the lts-landed-on-v4.x label Oct 8, 2015

jasnell mentioned this pull request Oct 8, 2015
Release proposal: v4.2.0 'Argon' (LTS) #3258
Closed

MylesBorins removed the land-on-v4.x label Nov 17, 2015

Uh oh!

src: add process.versions.icu#3102

src: add process.versions.icu #3102

Uh oh!

Conversation

evanlucas commented Sep 28, 2015

Uh oh!

bnoordhuisSep 28, 2015

Choose a reason for hiding this comment

Uh oh!

evanlucasSep 28, 2015

Choose a reason for hiding this comment

Uh oh!

srl295Sep 28, 2015 via email

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Sep 28, 2015

Uh oh!

evanlucas commented Sep 28, 2015

Uh oh!

evanlucas commented Sep 28, 2015

Uh oh!

srl295 commented Sep 28, 2015

Uh oh!

orangemocha commented Sep 28, 2015

Uh oh!

evanlucas commented Sep 28, 2015

Uh oh!

Fishrock123 commented Sep 28, 2015

Uh oh!

silverwind commented Sep 28, 2015

Uh oh!

evanlucas commented Sep 28, 2015

Uh oh!

silverwindSep 28, 2015

Choose a reason for hiding this comment

Uh oh!

jbergstroemSep 28, 2015

Choose a reason for hiding this comment

Uh oh!

silverwindSep 28, 2015

Choose a reason for hiding this comment

Uh oh!

jbergstroemSep 28, 2015

Choose a reason for hiding this comment

Uh oh!

silverwind commented Sep 28, 2015

Uh oh!

srl295 commented Sep 28, 2015

Uh oh!

evanlucas commented Sep 29, 2015

Uh oh!

rvagg commented Sep 29, 2015

Uh oh!

rvagg commented Sep 29, 2015

Uh oh!

evanlucas commented Sep 29, 2015

Uh oh!

evanlucas commented Sep 29, 2015

Uh oh!

silverwind commented Sep 29, 2015

Uh oh!

evanlucas commented Sep 29, 2015

Uh oh!

silverwind commented Sep 29, 2015

Uh oh!

evanlucas commented Sep 29, 2015

Uh oh!

jbergstroem commented Sep 29, 2015

Uh oh!

MylesBorins commented Nov 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants