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.

Sign up for GitHub

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

Jump to bottom

src: add process.versions.icu #3102

Merged
merged 1 commit into from
Sep 29, 2015
Merged

src: add process.versions.icu #3102

merged 1 commit into from
Sep 29, 2015

Conversation

evanlucas
Copy link
Contributor

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

Fixes: #3089

R= @silverwind / @srl295 ?

@evanlucas 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
bnoordhuis reviewed Sep 28, 2015
View reviewed changes
src/node.cc
@@ -17,6 +17,7 @@

#if defined(NODE_HAVE_I18N_SUPPORT)
#include "node_i18n.h"
#include <unicode/putil.h>
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be unicode/uversion.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good call

Copy link
Member

@srl295 srl295 Sep 28, 2015 via email

Choose a reason for hiding this comment

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

@evanlucas
Copy link
Contributor Author

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

@evanlucas
Copy link
Contributor Author

Ah, linter wasn't happy. Fixed

@evanlucas
Copy link
Contributor Author

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

@srl295
Copy link
Member

srl295 commented Sep 28, 2015

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

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
Copy link
Contributor Author

Updated to use unicode/uvernum.h

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

@Fishrock123
Copy link
Contributor

SGTM

@silverwind
Copy link
Contributor

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

@evanlucas
Copy link
Contributor Author

Updated docs as well

silverwind
silverwind reviewed Sep 28, 2015
View reviewed changes
doc/api/process.markdown
@@ -648,6 +648,7 @@ Will print something like:
zlib: '1.2.8',
ares: '1.10.0-DEV',
modules: '43',
icu: '55.1', // only if built with i18n support
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@silverwind
Copy link
Contributor

LGTM

@srl295
Copy link
Member

srl295 commented Sep 28, 2015

LGTM here also

@evanlucas
Copy link
Contributor Author

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

@rvagg
Copy link
Member

rvagg commented Sep 29, 2015

lgtm

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

rvagg commented Sep 29, 2015

tagged as semver-minor, any objections?

@evanlucas
Copy link
Contributor Author

works for me :]

@evanlucas
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 <srloomis@us.ibm.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Rod Vagg <rod@vagg.org>
@evanlucas evanlucas closed this Sep 29, 2015
@evanlucas evanlucas deleted the addicu branch September 29, 2015 13:25
@evanlucas evanlucas merged commit 30b8bb0 into nodejs:master Sep 29, 2015
@evanlucas
Copy link
Contributor Author

Thanks! Landed in 30b8bb0

@silverwind
Copy link
Contributor

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

@evanlucas
Copy link
Contributor Author

The branch for this one was addicu

$ git remote -v
mine     git@github.com:evanlucas/io.js (fetch)
mine     git@github.com:evanlucas/io.js (push)
origin   git@github.com:nodejs/io.js (fetch)
origin   git@github.com:nodejs/io.js (push)

$ git push mine +HEAD:addicu && git push origin master && git push mine :addicu

@silverwind
Copy link
Contributor

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

@evanlucas
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

@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.

@rvagg rvagg mentioned this pull request Sep 30, 2015
Closed
@jasnell jasnell mentioned this pull request Oct 8, 2015
Closed
29 tasks
evanlucas added a commit that referenced this pull request Oct 8, 2015
@evanlucas @jasnell
src: add process.versions.icu
7271cb0 
If i18n support is present, add the icu version
to process.versions

Fixes: #3089
PR-URL: #3102
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Rod Vagg <rod@vagg.org>
@jasnell jasnell mentioned this pull request Oct 8, 2015
Closed
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-08, Version 4.2.0 'Argon' (LTS) Release
08c0aed 
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
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-08, Version 4.2.0 'Argon' (LTS) Release
9fd6904 
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
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-12, Version 4.2.0 'Argon' (LTS) Release
6dc12b1 
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
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-12, Version 4.2.0 'Argon' (LTS) Release
47db247 
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
Copy link
Contributor

landed in v4.x-staging in 7271cb0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@evanlucas @srl295 @orangemocha @Fishrock123 @silverwind @rvagg @jbergstroem @MylesBorins @bnoordhuis @jasnell