-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: add docu for server.address() for pipe case #12907
Conversation
Add documentation for net.server.address() for the case it is listening on a pipe or unix domain socket instead an IP socket. Fixes: #12895
doc/api/net.md
Outdated
`{ port: 12346, family: 'IPv4', address: '127.0.0.1' }` | ||
`{ port: 12346, family: 'IPv4', address: '127.0.0.1' }`. | ||
|
||
For servers listening on a pipe or unix domain sockets the name is returned |
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.
Nit: Decide whether you want singular or plural and use it for both. pipe
+ socket
or else pipes
+ sockets
Nit: unix
-> UNIX
Nit: comma between socket
(or sockets
) and the
.
doc/api/net.md
Outdated
`{ port: 12346, family: 'IPv4', address: '127.0.0.1' }`. | ||
|
||
For servers listening on a pipe or unix domain sockets the name is returned | ||
as string. |
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.
Nit: as a string.
doc/api/net.md
Outdated
Useful to find which port was assigned when getting an OS-assigned address. | ||
Returns an object with `port`, `family`, and `address` properties: | ||
`{ port: 12346, family: 'IPv4', address: '127.0.0.1' }` | ||
`{ port: 12346, family: 'IPv4', address: '127.0.0.1' }`. |
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.
Nit: Not sure why the period was added but I don't think it should be there. /cc @nodejs/documentation
Thanks for the improvement! I left a bunch of nits--hope that isn't discouraging. Happens a lot with documentation. They can be ignored if you want and/or they can be applied by whoever chooses to land this. |
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.
Looks good with @Trott’s suggestions, thank you!
Thanks for the review! Added changes. |
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.
Nit: You may want to change the subsystem for this change as doc:
, instead of net:
. Thanks for the patch @Flarna 🙂
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.
LGTM
Looks like @Trott's comments are addressed CI run: https://ci.nodejs.org/job/node-test-pull-request/7990/ |
CI failures on windows are machine issue, not related to change, so CI looks good. |
Add documentation for net.server.address() for the case it is listening on a pipe or unix domain socket instead an IP socket. PR-URL: #12907 Fixes: #12895 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Landed in 13487c4 |
Add documentation for net.server.address() for the case it is listening on a pipe or unix domain socket instead an IP socket. PR-URL: nodejs#12907 Fixes: nodejs#12895 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add documentation for net.server.address() for the case it is listening on a pipe or unix domain socket instead an IP socket. PR-URL: #12907 Fixes: #12895 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Issue was raised on v6.x, so backporting to there. |
Add documentation for net.server.address() for the case it is listening on a pipe or unix domain socket instead an IP socket. PR-URL: #12907 Fixes: #12895 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add documentation for net.server.address() for the case it is listening
on a pipe or unix domain socket instead an IP socket.
Fixes: #12895
Checklist
Affected core subsystem(s)
doc