dns: use isIp consistently #5804
Conversation
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
|
LGTM if CI is green |
|
You might get some push back because a lot of this is style only. That said, dropping |
|
LGTM. |
|
@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 :) |
|
LGTM if CI is happy |
|
LGTM |
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>
|
Landed in b929988 |
|
Thanks :) |
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
|
this change is not landing cleanly on v4 due to @benjamingr would you be able to manually backport to v4 and send a PR? |
|
Sure |
|
@benjamingr ping 😄 |
|
@benjamingr ping |
|
@benjamingr hey buddy... hows it going? |
|
I totally forgot about this again - so not good :D |
|
@benjamingr hows it going :P |
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
netanduses both of them both throughout the code base. This PR removes the
direct dependency
dnshas onnetand usesisIpfrom c-ares allthe time. Note that both functions do the same thing.