-
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.lookup return a string with no options #11334
Conversation
If all is false (default) the return value is not an array, using a pluralized variable name is confusing
dns.lookup('iana.org', (err, addresses, family) => { | ||
console.log('addresses:', addresses); | ||
dns.lookup('iana.org', (err, address, family) => { | ||
console.log('address:', address); |
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.
It would likely be worthwhile showing two examples, this one and one with all = true
so that address
becomes an Array.
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.
this PR fix a mistake, not to enhance the documentation, I can do it but I'm scared that we end up on a very long debate on when to provide examples or not instead of a 5 seconds fix....
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.
Fix: nodejs#11334 PR-URL: nodejs#11350 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Fix: nodejs#11334 PR-URL: nodejs#11350 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
I'd like this to land as-is. It has two approvals so it should be land-able. |
@dcharbonnier Ooops, didn't notice that it was the OP that closed it. Uh, yeah, if you'd prefer to close it again, I won't re-open it. |
Rebased and there are no changes, so it looks like this change was handled (probably by another author?) in a different PR. Closing again. Sorry for the noise, everyone. |
If all is false (default) the return value is not an array, using a pluralized variable name is confusing