FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: add process.versions.icu by evanlucas · Pull Request #3102 · nodejs/node · GitHub

/ node Public

src: add process.versions.icu - #3102

Merged
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:addicu
Sep 29, 2015
ConversationCommits (1)ChecksFiles changed
Merged

src: add process.versions.icu#3102
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:addicu

Conversation

evanlucas commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

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

Fixes: #3089

R= @silverwind / @srl295 ?

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
Comment thread src/node.cc Outdated

bnoordhuis Sep 28, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this should be unicode/uversion.h?

evanlucas Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah good call

srl295 Sep 28, 2015 via email

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

evanlucas commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

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

evanlucas commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

Ah, linter wasn't happy. Fixed

evanlucas commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

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

srl295 commented Sep 28, 2015

Copy link
Copy Markdown
Member

.. 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 commented Sep 28, 2015

Copy link
Copy Markdown
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 commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

Updated to use unicode/uvernum.h

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

Fishrock123 commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

SGTM

silverwind commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

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

evanlucas commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

Updated docs as well

silverwind reviewed Sep 28, 2015
View reviewed changes
Comment thread doc/api/process.markdown Outdated

silverwind Sep 28, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jbergstroem Sep 28, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind Sep 28, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jbergstroem Sep 28, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

LGTM

srl295 commented Sep 28, 2015

Copy link
Copy Markdown
Member

LGTM here also

evanlucas commented Sep 29, 2015

Copy link
Copy Markdown
Contributor Author

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

rvagg commented Sep 29, 2015

Copy link
Copy Markdown
Member

lgtm

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

rvagg commented Sep 29, 2015

Copy link
Copy Markdown
Member

tagged as semver-minor, any objections?

evanlucas commented Sep 29, 2015

Copy link
Copy Markdown
Contributor Author

works for me :]

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 closed this Sep 29, 2015
evanlucas deleted the addicu branch September 29, 2015 13:25
evanlucas merged commit 30b8bb0 into nodejs:master Sep 29, 2015

evanlucas commented Sep 29, 2015

Copy link
Copy Markdown
Contributor Author

Thanks! Landed in 30b8bb0

silverwind commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

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

evanlucas commented Sep 29, 2015

Copy link
Copy Markdown
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 commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

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

evanlucas commented Sep 29, 2015

Copy link
Copy Markdown
Contributor Author

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

jbergstroem commented Sep 29, 2015

Copy link
Copy Markdown
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 mentioned this pull request Sep 30, 2015
Closed
jasnell mentioned this pull request Oct 8, 2015
Closed
29 tasks
evanlucas added a commit that referenced this pull request Oct 8, 2015
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 mentioned this pull request Oct 8, 2015
Closed
jasnell added a commit that referenced this pull request Oct 12, 2015
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
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
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
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 commented Nov 17, 2015

Copy link
Copy Markdown
Contributor

landed in v4.x-staging in 7271cb0

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL