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

url: use hasIntl instead of try-catch #11571

Closed
wants to merge 1 commit into from
Closed

url: use hasIntl instead of try-catch #11571

wants to merge 1 commit into from

Conversation

watilde
Copy link
Member

@watilde watilde commented Feb 27, 2017

Like the other lib codes, we should use process.binding('config').hasIntl instead of try-catch to make sure icu is bonded or not.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Feb 27, 2017
@JacksonTian
Copy link
Contributor

LGTM

@TimothyGu
Copy link
Member

TimothyGu commented Feb 27, 2017 •
edited
Loading

Slight nit in commit message: IMO "internal modules" sounds better than "lib codes". Otherwise LGTM.

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

@gibfahn
Copy link
Member

gibfahn commented Feb 27, 2017

Not entirely sure what happened with linuxone in that last CI, rerunning:

CI 2: https://ci.nodejs.org/job/node-test-commit/8137/

EDIT: Passed on rerun

@watilde
Copy link
Member Author

watilde commented Feb 28, 2017

I will update the commit message to replace lib codes with internal modules :)

@watilde
url: use hasIntl instead of try-catch
50e7e6e 
Like the other internal modules, we should use
`process.binding('config').hasIntl` instead of `try-catch`
to make sure `icu` is bonded or not.
@watilde
Copy link
Member Author

watilde commented Mar 3, 2017

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

jasnell pushed a commit that referenced this pull request Mar 4, 2017
@watilde @jasnell
url: use hasIntl instead of try-catch
cccc6d8 
Like the other internal modules, we should use
`process.binding('config').hasIntl` instead of `try-catch`
to make sure `icu` is bonded or not.

PR-URL: #11571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 4, 2017

Landed in cccc6d8

@jasnell jasnell closed this Mar 4, 2017
@watilde watilde deleted the feature/url-hasIntl branch March 4, 2017 16:15
addaleax pushed a commit that referenced this pull request Mar 5, 2017
@watilde @addaleax
url: use hasIntl instead of try-catch
24e6fcc 
Like the other internal modules, we should use
`process.binding('config').hasIntl` instead of `try-catch`
to make sure `icu` is bonded or not.

PR-URL: #11571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@watilde @MylesBorins
url: use hasIntl instead of try-catch
e6c65b2 
Like the other internal modules, we should use
`process.binding('config').hasIntl` instead of `try-catch`
to make sure `icu` is bonded or not.

PR-URL: #11571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@watilde @MylesBorins
url: use hasIntl instead of try-catch
6344997 
Like the other internal modules, we should use
`process.binding('config').hasIntl` instead of `try-catch`
to make sure `icu` is bonded or not.

PR-URL: #11571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@watilde @andrew749
url: use hasIntl instead of try-catch
ae48e46 
Like the other internal modules, we should use
`process.binding('config').hasIntl` instead of `try-catch`
to make sure `icu` is bonded or not.

PR-URL: nodejs/node#11571
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@watilde @JacksonTian @TimothyGu @gibfahn @jasnell @addaleax @lpinca @cjihrig @MylesBorins @nodejs-github-bot