Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

url: use hasIntl instead of try-catch #11571

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
Closed
watilde wants to merge 1 commit into from
Closed

url: use hasIntl instead of try-catch #11571

watilde 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

JacksonTian commented Feb 27, 2017

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 watilde force-pushed the feature/url-hasIntl branch from 15cf5ba to 50e7e6e Compare February 28, 2017 09:06
@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

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.