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

dns: use isIp consistently by benjamingr · Pull Request #5804 · nodejs/node · GitHub

/ node Public

dns: use isIp consistently - #5804

Closed
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:dns-use-isip-consistently
ConversationCommits (1)ChecksFiles changed
Closed

dns: use isIp consistently#5804
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:dns-use-isip-consistently

Conversation

benjamingr commented Mar 19, 2016

Copy link
Copy Markdown
Member

Affected core subsystem(s)

dns

Description of change

This is a more modular take on #5762 ,

Currently the DNS module imports isIP from both cares and net and
uses both of them both throughout the code base. This PR removes the
direct dependency dns has on net and uses isIp from c-ares all
the time. Note that both functions do the same thing.

benjamingr added the dns Issues and PRs related to the dns subsystem. label Mar 19, 2016
dns: use isIp consistently
65acb8d 
Currently the DNS module imports isIP from both cares and `net` and
uses both of them both throughout the code base. This PR removes the
direct dependency `dns` has on `net` and uses `isIp` from c-ares all
the time. Note that both functions do the same thing.

PR-URL:
Reviewed-By:
Reviewed-By:
benjamingr force-pushed the dns-use-isip-consistently branch from 1eb87bf to 65acb8d Compare March 19, 2016 19:56

Trott commented Mar 19, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green

benjamingr commented Mar 19, 2016

Copy link
Copy Markdown
Member Author

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

cjihrig commented Mar 20, 2016

Copy link
Copy Markdown
Contributor

You might get some push back because a lot of this is style only. That said, dropping net LGTM.

thefourtheye commented Mar 20, 2016

Copy link
Copy Markdown
Contributor

LGTM.

benjamingr commented Mar 20, 2016

Copy link
Copy Markdown
Member Author

@cjihrig well, this specific change isn't style only (it's just dropping the dependency). As for the others, I don't consider the push-back negative, as long as people are discussing it and we can reach consensus and improve the code base I'm all for it (and I know this requires some extra work and writing a lot of code that'll never make it in and I'm fine with that too). Thanks for the concern :)

evanlucas commented Mar 20, 2016

Copy link
Copy Markdown
Contributor

LGTM if CI is happy

jasnell commented Mar 21, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell pushed a commit that referenced this pull request Mar 21, 2016
dns: use isIp consistently
b929988 
Currently the DNS module imports isIP from both cares and `net` and
uses both of them both throughout the code base. This PR removes the
direct dependency `dns` has on `net` and uses `isIp` from c-ares all
the time. Note that both functions do the same thing.

PR-URL: #5804
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Mar 21, 2016

Copy link
Copy Markdown
Member

Landed in b929988

jasnell closed this Mar 21, 2016

benjamingr commented Mar 21, 2016

Copy link
Copy Markdown
Member Author

Thanks :)

benjamingr mentioned this pull request Mar 22, 2016
Closed
Fishrock123 mentioned this pull request Mar 22, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
dns: use isIp consistently
3fef69b 
Currently the DNS module imports isIP from both cares and `net` and
uses both of them both throughout the code base. This PR removes the
direct dependency `dns` has on `net` and uses `isIp` from c-ares all
the time. Note that both functions do the same thing.

PR-URL: #5804
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Conflicts:
	lib/dns.js

MylesBorins commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

this change is not landing cleanly on v4 due to @benjamingr would you be able to manually backport to v4 and send a PR?

benjamingr commented Mar 31, 2016

Copy link
Copy Markdown
Member Author

Sure

MylesBorins commented Apr 8, 2016

Copy link
Copy Markdown
Contributor

@benjamingr ping 😄

MylesBorins commented May 17, 2016

Copy link
Copy Markdown
Contributor

@benjamingr ping

MylesBorins commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

@benjamingr hey buddy... hows it going?

benjamingr commented Jun 2, 2016

Copy link
Copy Markdown
Member Author

I totally forgot about this again - so not good :D

MylesBorins commented Aug 30, 2016

Copy link
Copy Markdown
Contributor

@benjamingr hows it going :P

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

dns Issues and PRs related to the dns subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL