-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
dns: use template literals #5809
dns: use template literals #5809
Conversation
Ping @Trott for making sure this change is aligned with what Trott had in mind. |
This is not my interpretation of moving incrementally, but hey. |
I'm +0 but LGTM |
@Fishrock123 elaborate please? I'm not tied to any of the DNS PRs and would like to understand better what would make better contributions. |
@@ -25,7 +25,7 @@ function errnoException(err, syscall, hostname) { | |||
} | |||
var ex = null; | |||
if (typeof err === 'string') { // c-ares error code. | |||
ex = new Error(syscall + ' ' + err + (hostname ? ' ' + hostname : '')); | |||
ex = new Error(`${syscall} ${err}${(hostname ? ' ' + hostname : '')}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this will still trigger a "unexpected string concatenation" in ESLint because of the ' ' + hostname
part.
Yes, that's basically what I had in mind, although I left a comment on one of them. (Feel free to come up with your own solution there, or no solution at all, but probably, the ternary should be moved to its own line and set to variable which can be included in the template literal. That will improve readability, which is basically what the whole change-to-template-literal thing is about.) Again, not sure if this is something that will gain consensus or not, but I certainly like it and I'm optimistic. Some parts of the code are easier to convert to template literals than others, though. If you're brave, take a crack at |
Prefer the use of template string literals over string concatenation in the dns module, makes dns consistent with other modules basically doing nodejs#5778 for it. PR-URL: nodejs#5809 Reviewed-By: James M Snell <jasnell@gmail.com>
8a70021
to
74174d6
Compare
Landed in 8baaa25 On another note, I just realized Sundays are weekend in the US (not the case here) - since it's a trivial change I'm not sure this is a big deal but if anyone feels strongly about this I'll revert it and land it again tomorrow in case people haven't had the chance to look into it. Cheers |
Affected core subsystem(s)
dns
Description of change
Prefer the use of template string literals over string concatenation in the dns module, makes dns consistent with other modules basically doing #5778 for it.
(ci https://ci.nodejs.org/job/node-test-pull-request/1976/ )