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

doc: clarify the callback arguments of dns.resolve #9532

Closed
wants to merge 6 commits into from

Conversation

silverwind
Copy link
Contributor

@silverwind silverwind commented Nov 9, 2016

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

doc

Description of change

Shortened and clarfied the callback arguments of dns.resolve. The fact that the MX rrtype was also returning a object was missing entirely.

@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 Nov 9, 2016
@silverwind silverwind force-pushed the dns-resolve-api branch 2 times, most recently from 477772c to df695fe Compare November 9, 2016 23:34
@silverwind silverwind changed the title doc: clarify return value of dns.resolve doc: clarify the callback arguments of dns.resolve Nov 9, 2016
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

A couple of nits, but LGTM

doc/api/dns.md Outdated
[`dns.resolveSoa()`][] method. The type of each item in `addresses` is
determined by the record type, and described in the documentation for the
corresponding lookup methods.
The `callback` function has arguments `(err, addresses)` and will receive an
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would retain the ". When successful, addreses will be" wording. The revised text says the callback "will receive an array" but it's not immediately clear that the array is addresses. That was clear in the previous wording.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I also think the new wording is a little ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I'll put the part about addresses back.

doc/api/dns.md Outdated
On error, `err` is an [`Error`][] object, where `err.code` is
one of the error codes listed [here](#dns_error_codes).
On error, `err` is an [`Error`][] object, where `err.code` is one of the error
codes listed [here](#dns_error_codes).
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Avoid here as link text. Maybe ...one of the error codes listed under [DNS error codes].?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about a concise where err.code is one of the [error codes].?

doc/api/dns.md Outdated
[`dns.resolveSoa()`][] method. The type of each item in `addresses` is
determined by the record type, and described in the documentation for the
corresponding lookup methods.
The `callback` function has arguments `(err, addresses)` and will receive an
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I also think the new wording is a little ambiguous.

doc/api/dns.md Outdated
[`dns.resolveSoa()`][] method. The type of each item in `addresses` is
determined by the record type, and described in the documentation for the
corresponding lookup methods.
`addresses` will be an array of results. A result will be a string except when
Copy link
Contributor

Choose a reason for hiding this comment

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

And the string will be.... what? Perhaps in the values enumeration above, it should describe the string format for the record types, and maybe even give an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IP addresses, hostnames (for CNAME) or text (for TXT). You think we should go into su much detail here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I read it, and if was going to use this API, I'd start off by calling it to see what it returns, because it doesn't say. So, yes!

@silverwind
Copy link
Contributor Author

silverwind commented Nov 10, 2016

I'll have to revise again, NAPTR queries also return objects. Maybe we should just link to the individual methods below in the list of types to avoid duplication?

@sam-github
Copy link
Contributor

Maybe we should just link to the individual methods below in the list of types to avoid duplication?

Excellent idea, yes, if each rtype maps to an API lower down, that describes the string/object format and gives an example, no need to duplicate, linking is great.

doc/api/dns.md Outdated
On error, `err` is an [`Error`][] object, where `err.code` is
one of the error codes listed [here](#dns_error_codes).
On error, `err` is an [`Error`][] object, where `err.code` is one of the
[error codes](#dns_error_codes).
Copy link
Member

Choose a reason for hiding this comment

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

Simply saying "err.code is one of the error codes" sounds vague. Perhaps specifying "DNS error codes" would be a bit better?

@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

@bnoordhuis ... has this been updated to your satisfaction?

@silverwind
Copy link
Contributor Author

This is still WIP!

@silverwind
Copy link
Contributor Author

silverwind commented Dec 9, 2016

Reworked this into a table and sorted everything alphabetically. There seems to be an issue that JS types in tables don't get linkified, so it renders like this right now for me:

doc/api/dns.md Outdated
[`dns.resolveSoa()`][] method. The type of each item in `addresses` is
determined by the record type, and described in the documentation for the
corresponding lookup methods.
`addresses` will be an array of results.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could call this results so it's consistent with the table above. After all, not all results are addresses.

@jasnell
Copy link
Member

jasnell commented Jan 11, 2017

ping @bnoordhuis

@silverwind
Copy link
Contributor Author

I'll look into fixing this type bug in the doctool, but would appreciate feedback on the table here!

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

ping @silverwind @bnoordhuis ... updates on this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@silverwind
Copy link
Contributor Author

Still got to investigate why {type} does not render correctly in tables.

@silverwind
Copy link
Contributor Author

Thanks! Landed in d33b3d1 and 0584aeb.

@silverwind silverwind closed this Apr 19, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
PR-URL: #9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
PR-URL: #9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

This doesn't land cleanly, could you backport to v6.x if appropriate?

@silverwind
Copy link
Contributor Author

Let's land #13054 on v6.x first, I think it may land cleanly then.

@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

Sadly it does not

Conflict on first commit:
++<<<<<<< HEAD
 +
 +Uses the DNS protocol to resolve a hostname (e.g. `'nodejs.org'`) into an
 +array of the record types specified by `rrtype`.
 +
 +Valid values for `rrtype` are:
 +
 + * `'A'` - IPV4 addresses, default
 + * `'AAAA'` - IPV6 addresses
 + * `'MX'` - mail exchange records
 + * `'TXT'` - text records
 + * `'SRV'` - SRV records
 + * `'PTR'` - PTR records
 + * `'NS'` - name server records
 + * `'CNAME'` - canonical name records
 + * `'SOA'` - start of authority record
 + * `'NAPTR'` - name authority pointer record
 +
 +The `callback` function has arguments `(err, addresses)`. When successful,
 +`addresses` will be an array, except when resolving an SOA record which returns
 +an object structured in the same manner as one returned by the
 +[`dns.resolveSoa()`][] method. The type of each item in `addresses` is
 +determined by the record type, and described in the documentation for the
 +corresponding lookup methods.
 +
 +On error, `err` is an [`Error`][] object, where `err.code` is
 +one of the error codes listed [here](#dns_error_codes).
++=======
+ - `hostname` {string} Hostname to resolve.
+ - `rrtype` {string} Resource record type. Default: `'A'`.
+ - `callback` {Function}
+   - `err` {Error}
+   - `records` {string[] | Object[] | string[][] | Object}
+ 
+ Uses the DNS protocol to resolve a hostname (e.g. `'nodejs.org'`) into an array
+ of the resource records. The `callback` function has arguments
+ `(err, records)`. When successful, `records` will be an array of resource
+ records. The type and structure of individual results varies based on `rrtype`:
+ 
+ |  `rrtype` | `records` contains             | Result type | Shorthand method         |
+ |-----------|--------------------------------|-------------|--------------------------|
+ | `'A'`     | IPv4 addresses (default)       | {string}    | [`dns.resolve4()`][]     |
+ | `'AAAA'`  | IPv6 addresses                 | {string}    | [`dns.resolve6()`][]     |
+ | `'CNAME'` | canonical name records         | {string}    | [`dns.resolveCname()`][] |
+ | `'MX'`    | mail exchange records          | {Object}    | [`dns.resolveMx()`][]    |
+ | `'NAPTR'` | name authority pointer records | {Object}    | [`dns.resolveNaptr()`][] |
+ | `'NS'`    | name server records            | {string}    | [`dns.resolveNs()`][]    |
+ | `'PTR'`   | pointer records                | {string}    | [`dns.resolvePtr()`][]   |
+ | `'SOA'`   | start of authority records     | {Object}    | [`dns.resolveSoa()`][]   |
+ | `'SRV'`   | service records                | {Object}    | [`dns.resolveSrv()`][]   |
+ | `'TXT'`   | text records                   | {string}    | [`dns.resolveTxt()`][]   |
+ 
+ On error, `err` is an [`Error`][] object, where `err.code` is one of the
+ [DNS error codes](#dns_error_codes).
++>>>>>>> d33b3d1086... doc: clarify the callback arguments of dns.resolve

Would you be willing to backport?

@silverwind
Copy link
Contributor Author

Backport in #13073

silverwind added a commit to silverwind/node that referenced this pull request Jun 19, 2017
PR-URL: nodejs#9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
silverwind added a commit to silverwind/node that referenced this pull request Jun 19, 2017
PR-URL: nodejs#9532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #9532
Backport-PR-URL: #13073
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #9532
Backport-PR-URL: #13073
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #9532
Backport-PR-URL: #13073
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #9532
Backport-PR-URL: #13073
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants