doc: argument types for dns methods #11764

ameliavoncat · 2017-03-09T05:45:17Z

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

addaleax

Once more, thanks for doing this! ❤

addaleax · 2017-03-09T16:41:31Z

doc/api/dns.md

-
-With the `all` option set to `true`, the arguments change to
+AAAA (IPv6) record. All `option` properties are optional. If `options` is an
+integer, then it must be `4` or `6`- if `options` is not provided, then IPv4


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

addaleax · 2017-03-09T16:42:14Z

doc/api/dns.md

+- `callback(err, hostname, service)` {Function}
+  - `err` {Error}
+  - `hostname` {string} Eg: `localhost`
+  - `service` {string} Eg: `http`


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

addaleax · 2017-03-09T16:44:57Z

doc/api/dns.md


 Uses the DNS protocol to resolve a start of authority record (`SOA` record) for
-the `hostname`. The `addresses` argument passed to the `callback` function will
+the `hostname`. The `address` argument passed to the `callback` function will


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

Updated. 😸

sam-github

Nice! Thanks.

sam-github · 2017-03-09T18:10:58Z

doc/api/dns.md

+- `hostname` {string}
+- `options` {integer | Object}
+  - `family` {integer} The record family. Must be `4` or `6`. IPv4
+    and v6 addresses are both valid by default.


"both returned by default"?

sam-github · 2017-03-09T18:12:29Z

doc/api/dns.md

-With the `all` option set to `true`, the arguments change to
+AAAA (IPv6) record. All `option` properties are optional. If `options` is an
+integer, then it must be `4` or `6` – if `options` is not provided, then IPv4
+and IPv6 addresses are both valid.


valid -> "returned if found"?

sam-github · 2017-03-09T18:12:56Z

doc/api/dns.md

+- `port` {number}
+- `callback(err, hostname, service)` {Function}
+  - `err` {Error}
+  - `hostname` {string} e.g. `localhost`


'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 · 2017-03-09T18:51:18Z

doc/api/dns.md

@@ -147,6 +139,12 @@ on some operating systems (e.g FreeBSD 10.1).
 <!-- YAML
 added: v0.11.14
 -->
+- `address` {string}
+- `port` {number}
+- `callback(err, hostname, service)` {Function}


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

TimothyGu · 2017-03-09T18:52:09Z

doc/api/dns.md

+- `rrtype` {string}
+- `callback(err, addresses)` {Function}
+  - `err` {Error}
+  - `addresses` {Array}


Include the type of array members also

The type of the contents of this array depend on rrtype

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

ameliavoncat · 2017-03-09T19:18:41Z

Updated.

TimothyGu

LGTM other than the nits.

TimothyGu · 2017-03-09T21:52:41Z

doc/api/dns.md

+- `rrtype` {string}
+- `callback` {Function}
+  - `err` {Error}
+  - `addresses` {string[] | Object[] | Array[string[]] | Object}


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

TimothyGu · 2017-03-09T21:52:49Z

doc/api/dns.md

+- `hostname` {string}
+- `callback` {Function}
+  - `err` {Error}
+  - `addresses` {Array[string[]]}


ameliavoncat · 2017-03-09T22:29:23Z

Nits fixed. 👍

jasnell

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

jasnell · 2017-03-10T04:58:22Z

doc/api/dns.md

+- `hostname` {string} Hostname to resolve.
+- `options` {Object}
+  - `ttl` {boolean} Retrieve the Time-To-Live value (TTL) of each record.
+    When `true`, the callback receives an array of `{ address: '1.2.3.4', ttl: 60 }` objects


long line here :-)

jasnell · 2017-03-10T04:58:36Z

doc/api/dns.md

+- `hostname` {string} Hostname to resolve.
+- `options` {Object}
+  - `ttl` {boolean} Retrieve the Time-To-Live value (TTL) of each record.
+    When `true`, the callback receives an array of `{ address: '0:1:2:3:4:5:6:7', ttl: 60 }` objects


... and here

mhdawson

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

Refs: nodejs#9399

ameliavoncat · 2017-03-10T22:53:57Z

Fixed the long lines. :)

Refs: #9399 PR-URL: #11764 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2017-03-15T00:32:03Z

Landed in 1faf136

Refs: nodejs#9399 PR-URL: nodejs#11764 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-04-18T17:13:30Z

would anyone like to backport to v6.x?

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 force-pushed the argument-types-for-dns branch 2 times, most recently from 14835e3 to ceb43e9 Compare March 9, 2017 06:01

addaleax approved these changes Mar 9, 2017

View reviewed changes

ameliavoncat force-pushed the argument-types-for-dns branch from ceb43e9 to afaf206 Compare March 9, 2017 17:23

addaleax approved these changes Mar 9, 2017

View reviewed changes

sam-github reviewed Mar 9, 2017

View reviewed changes

TimothyGu reviewed Mar 9, 2017

View reviewed changes

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

ameliavoncat force-pushed the argument-types-for-dns branch from 8952c1e to eea3261 Compare March 9, 2017 20:04

TimothyGu approved these changes Mar 9, 2017

View reviewed changes

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

jasnell approved these changes Mar 10, 2017

View reviewed changes

mhdawson approved these changes Mar 10, 2017

View reviewed changes

cjihrig approved these changes Mar 10, 2017

View reviewed changes

doc: argument types for dns methods

73ab559

Refs: nodejs#9399

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

jasnell closed this Mar 15, 2017

italoacasas mentioned this pull request Mar 20, 2017

v7.7.4 Release Proposal #11941

Merged

MylesBorins added the backported-to-v6.x label Apr 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: argument types for dns methods #11764

doc: argument types for dns methods #11764

ameliavoncat commented Mar 9, 2017

addaleax left a comment

addaleax Mar 9, 2017

addaleax Mar 9, 2017

addaleax Mar 9, 2017

ameliavoncat Mar 9, 2017

sam-github left a comment

sam-github Mar 9, 2017

sam-github Mar 9, 2017

sam-github Mar 9, 2017

sam-github Mar 9, 2017

TimothyGu Mar 9, 2017

TimothyGu Mar 9, 2017

ameliavoncat Mar 9, 2017

ameliavoncat Mar 9, 2017

ameliavoncat commented Mar 9, 2017

TimothyGu left a comment

TimothyGu Mar 9, 2017

TimothyGu Mar 9, 2017

ameliavoncat commented Mar 9, 2017

jasnell left a comment

jasnell Mar 10, 2017

jasnell Mar 10, 2017

mhdawson left a comment

ameliavoncat commented Mar 10, 2017

jasnell commented Mar 15, 2017

MylesBorins commented Apr 18, 2017

doc: argument types for dns methods #11764

doc: argument types for dns methods #11764

Conversation

ameliavoncat commented Mar 9, 2017

Checklist

Affected core subsystem(s)

Description of changes

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameliavoncat commented Mar 9, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameliavoncat commented Mar 9, 2017

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

ameliavoncat commented Mar 10, 2017

jasnell commented Mar 15, 2017

MylesBorins commented Apr 18, 2017