Skip to content

doc: argument types for dns methods #11764

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

doc: argument types for dns methods #11764

wants to merge 1 commit into from

Conversation

ameliavoncat
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

documentation

Description of changes

Adds argument data types to the docs for dns methods

Issue

@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 Mar 9, 2017
@ameliavoncat ameliavoncat force-pushed the argument-types-for-dns branch 2 times, most recently from 14835e3 to ceb43e9 Compare March 9, 2017 06:01
addaleax
addaleax approved these changes Mar 9, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Once more, thanks for doing this! ❤

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave a space after the `6`? Also, if you’re using a dash, here’s an en dash to copy: – 😄

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think we’ve pretty much settled on e.g. in the docs

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Hm, should it be `callback(err, address)` above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. 😸

@ameliavoncat ameliavoncat force-pushed the argument-types-for-dns branch from ceb43e9 to afaf206 Compare March 9, 2017 17:23
sam-github
sam-github reviewed Mar 9, 2017
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

Nice! Thanks.

doc/api/dns.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

"IPv6"

Copy link
Contributor

Choose a reason for hiding this comment

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

"both returned by default"?

doc/api/dns.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

valid -> "returned if found"?

doc/api/dns.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

'example.com' would make a better example, localhost is a unusual host, it isn't even required to be resolved by a DNS server

TimothyGu
TimothyGu reviewed Mar 9, 2017
View reviewed changes
doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's customary to include the arguments in this line. Rather if I recall correctly it's just `callback` {Function}.

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Include the type of array members also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type of the contents of this array depend on rrtype

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to {string[] | Object[] | Array[string[]] | Object} to cover the possible types.

@ameliavoncat ameliavoncat force-pushed the argument-types-for-dns branch from afaf206 to 8952c1e Compare March 9, 2017 19:17
@ameliavoncat
Copy link
Contributor Author

Updated.

@ameliavoncat ameliavoncat force-pushed the argument-types-for-dns branch from 8952c1e to eea3261 Compare March 9, 2017 20:04
TimothyGu
TimothyGu approved these changes Mar 9, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM other than the nits.

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Array[string[]] is a bit ambiguous. string[][] should be better.

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@ameliavoncat ameliavoncat force-pushed the argument-types-for-dns branch from eea3261 to bc930b3 Compare March 9, 2017 22:27
@ameliavoncat
Copy link
Contributor Author

Nits fixed. 👍

jasnell
jasnell approved these changes Mar 10, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Generally LGTM, couple of long lines that need to be wrapped

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

long line here :-)

doc/api/dns.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

... and here

mhdawson
mhdawson approved these changes Mar 10, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

Other than the minor issues pointed out by the other reviewers, LGTM.

@ameliavoncat ameliavoncat force-pushed the argument-types-for-dns branch from bc930b3 to 73ab559 Compare March 10, 2017 22:52
@ameliavoncat
Copy link
Contributor Author

Fixed the long lines. :)

jasnell pushed a commit that referenced this pull request Mar 15, 2017
@ameliavoncat @jasnell
doc: argument types for dns methods
1faf136 
Refs: #9399
PR-URL: #11764
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 15, 2017

Landed in 1faf136

@jasnell jasnell closed this Mar 15, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
@ameliavoncat
doc: argument types for dns methods
78ca15d 
Refs: nodejs#9399
PR-URL: nodejs#11764
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 20, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@ameliavoncat @jungx098
doc: argument types for dns methods
97cf298 
Refs: nodejs#9399
PR-URL: nodejs#11764
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

would anyone like to backport to v6.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

+2 more reviewers

@sam-github sam-github sam-github left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ameliavoncat @jasnell @MylesBorins @sam-github @addaleax @TimothyGu @cjihrig @mhdawson @nodejs-github-bot