Skip to content
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

Closed
wants to merge 1 commit into from
Closed

dns.lookup return a string with no options #11334

wants to merge 1 commit into from

Conversation

dcharbonnier
Copy link
Contributor

If all is false (default) the return value is not an array, using a pluralized variable name is confusing

If all is false (default) the return value is not an array, using a pluralized variable name is confusing
@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations. labels Feb 13, 2017
dns.lookup('iana.org', (err, addresses, family) => {
console.log('addresses:', addresses);
dns.lookup('iana.org', (err, address, family) => {
console.log('address:', address);
Copy link
Member

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.

Copy link
Contributor Author

@dcharbonnier dcharbonnier Feb 13, 2017

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....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sam-github added a commit to sam-github/node that referenced this pull request Feb 16, 2017
Fix: nodejs#11334
PR-URL: nodejs#11350
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
Fix: nodejs#11334
PR-URL: nodejs#11350
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
Fix: #11334
PR-URL: #11350
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
Fix: #11334
PR-URL: #11350
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
Fix: #11334
PR-URL: #11350
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Fix: #11334
PR-URL: #11350
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Fix: #11334
PR-URL: #11350
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Aug 3, 2017

I'd like this to land as-is. It has two approvals so it should be land-able.

@Trott Trott reopened this Aug 3, 2017
@Trott
Copy link
Member

Trott commented Aug 3, 2017

@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.

@Trott
Copy link
Member

Trott commented Aug 3, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants