doc: fix inconsistent documentation of server.listen() #16020

mgjm · 2017-10-06T19:06:02Z

First commit:
The net, tls, http and https module have the same
server.listen() method, but have a different documenation.
Changed to a consistent link to the documentation of the net module.

Fixes #11915

Second commit:
The https.createServer() function is above some https.Server
methods. Move server.close() and server.listen() to the correct
position.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

The `net`, `tls`, `http` and `https` module have the same `server.listen()` method, but have a different documenation. Changed to a consistent link to the documentation of the `net` module.

The `https.createServer()` function is above some `https.Server` methods. Move `server.close()` and `server.listen()` to the correct position.

Trott · 2017-10-10T06:31:40Z

Ping @nodejs/documentation: Can we get some reviews for this?

joyeecheung · 2017-10-10T06:43:56Z

doc/api/http.md

-*Note*: The `server.listen()` method can be called again if and only if there was an error
-during the first `server.listen()` call or `server.close()` has been called.
-Otherwise, an `ERR_SERVER_ALREADY_LISTEN` error will be thrown.
+Start a server listening for connections. This method is identical to [`server.listen()`][] from [`net.Server`][].


I wouldn't put this as

This method is identical to [`server.listen()`][] from [`net.Server`][].

because it starts a HTTP server, not a bare TCP/IPC server. IMO a better way to put this would be something like:

Starts the HTTP server listening for connections. This method has the same signatures as [`server.listen()`][] from [`net.Server`][].

Actually the two methods are equal net.Server.prototype.listen === http.Server.prototype.listen. So a compromise would be:

Starts the HTTP server listening for connections. This method is identical to [`server.listen()`][] from [`net.Server`][].

joyeecheung · 2017-10-10T06:47:46Z

doc/api/https.md

+-->
+- `callback` {Function}
+
+See [`http.close()`][] for details.


http.close() is a little bit ambiguous. Maybe

See [`server.close()`][`http.close()`] from the HTTP module for details.

I only moved the description of http.close() to another location. But it would be a nice change to make the description better.

lpinca · 2017-10-14T07:51:13Z

@mgjm can you please address @joyeecheung comments?. Also please break lines at 80 chars.

The different server.listen() methods are all the same function but result in different servers listening for connections. This fact is now also documented.

The documentation of `server.close()` in the https module only links to the documentation in the http module and is a bit ambiguous. Changed the documentation in the https module to clarify the relation.

mgjm · 2017-10-14T15:20:02Z

Sorry for the delay.
I just made the changes according to my comments and pushed the changes to my branch.
Do i need to create a new pull request or something else?

lpinca · 2017-10-14T15:35:24Z

@mgjm no this is perfect.

lpinca · 2017-10-14T15:42:45Z

@bengl @sam-github @joyeecheung PTAL.

thefourtheye · 2017-10-15T10:58:22Z

doc/api/http.md

@@ -1963,6 +1888,7 @@ const req = http.request(options, (res) => {
 [`net.Server.listen(path)`]: net.html#net_server_listen_path_backlog_callback
 [`net.Server.listen(port)`]: net.html#net_server_listen_port_host_backlog_callback
 [`net.Server`]: net.html#net_class_net_server
+[`server.listen()`]: net.html#net_server_listen


All of these should be in the sorted order.

I am not certain if there is a lint rule for things like this but this would be pretty neat!

I would like to address this in another PR and write some automation / linting to check for the order of the documented methods as well as the order of the links

bengl · 2017-10-18T20:53:05Z

still LGTM

The `net`, `tls`, `http` and `https` module have the same `server.listen()` method, but have a different documenation. Changed to a consistent link to the documentation of the `net` module. PR-URL: #16020 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

BridgeAR · 2017-10-19T04:31:00Z

Landed in 278f653

Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 !

MylesBorins · 2017-10-23T22:16:50Z

hey @nodejs/collaborators would someone be willing to backport this to v8.x-staging?

The `net`, `tls`, `http` and `https` module have the same `server.listen()` method, but have a different documenation. Changed to a consistent link to the documentation of the `net` module. PR-URL: nodejs#16020 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

The `net`, `tls`, `http` and `https` module have the same `server.listen()` method, but have a different documenation. Changed to a consistent link to the documentation of the `net` module. PR-URL: nodejs/node#16020 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

The `net`, `tls`, `http` and `https` module have the same `server.listen()` method, but have a different documenation. Changed to a consistent link to the documentation of the `net` module. PR-URL: #16020 Backport-PR-URL: #16435 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2017-11-16T21:35:21Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

The `net`, `tls`, `http` and `https` module have the same `server.listen()` method, but have a different documenation. Changed to a consistent link to the documentation of the `net` module. PR-URL: nodejs/node#16020 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

codebytere · 2018-06-06T20:06:01Z

Currently working through some backports & since this appears to have been backported to v8.x already we may want to remove the backport-requested-v8.x label :)

mgjm added 2 commits October 6, 2017 11:50

doc: fix inconsistent documentation of server.listen()

d5e1492

The `net`, `tls`, `http` and `https` module have the same `server.listen()` method, but have a different documenation. Changed to a consistent link to the documentation of the `net` module.

doc: fixed order in https module

6877211

The `https.createServer()` function is above some `https.Server` methods. Move `server.close()` and `server.listen()` to the correct position.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 6, 2017

mscdex added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. tls Issues and PRs related to the tls subsystem. labels Oct 6, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 7, 2017

bengl approved these changes Oct 10, 2017

View reviewed changes

joyeecheung reviewed Oct 10, 2017

View reviewed changes

sam-github approved these changes Oct 10, 2017

View reviewed changes

mgjm added 2 commits October 14, 2017 17:12

doc: fix inconsistent documentation of server.listen()

4f7cf5f

The different server.listen() methods are all the same function but result in different servers listening for connections. This fact is now also documented.

doc: change server.close in https

f9fbfc6

The documentation of `server.close()` in the https module only links to the documentation in the http module and is a bit ambiguous. Changed the documentation in the https module to clarify the relation.

lpinca approved these changes Oct 14, 2017

View reviewed changes

joyeecheung approved these changes Oct 14, 2017

View reviewed changes

thefourtheye reviewed Oct 15, 2017

View reviewed changes

jasnell approved these changes Oct 15, 2017

View reviewed changes

BridgeAR approved these changes Oct 18, 2017

View reviewed changes

BridgeAR self-assigned this Oct 18, 2017

BridgeAR closed this Oct 19, 2017

MylesBorins added the backport-requested-v8.x label Oct 23, 2017

MylesBorins added the backported-to-v8.x label Oct 23, 2017

lpinca mentioned this pull request Oct 24, 2017

[v8.x backport] doc: fix inconsistent server.listen documentation #16435

Closed

lance mentioned this pull request Oct 28, 2017

meta: label for pre-backport request #16570

Closed

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

apapirovski mentioned this pull request Apr 13, 2018

Document tls.Server#listen(path, callback) and possible other signatures #11915

Closed

addaleax removed the backport-requested-v8.x label Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: fix inconsistent documentation of server.listen() #16020

doc: fix inconsistent documentation of server.listen() #16020

mgjm commented Oct 6, 2017 •

edited

Loading

Trott commented Oct 10, 2017

joyeecheung Oct 10, 2017

mgjm Oct 14, 2017

joyeecheung Oct 10, 2017

mgjm Oct 14, 2017

lpinca commented Oct 14, 2017

mgjm commented Oct 14, 2017

lpinca commented Oct 14, 2017

lpinca commented Oct 14, 2017

thefourtheye Oct 15, 2017

BridgeAR Oct 18, 2017

mgjm Oct 18, 2017

bengl commented Oct 18, 2017

BridgeAR commented Oct 19, 2017

MylesBorins commented Oct 23, 2017

MylesBorins commented Nov 16, 2017

codebytere commented Jun 6, 2018

doc: fix inconsistent documentation of server.listen() #16020

doc: fix inconsistent documentation of server.listen() #16020

Conversation

mgjm commented Oct 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott commented Oct 10, 2017

joyeecheung Oct 10, 2017

Choose a reason for hiding this comment

mgjm Oct 14, 2017

Choose a reason for hiding this comment

joyeecheung Oct 10, 2017

Choose a reason for hiding this comment

mgjm Oct 14, 2017

Choose a reason for hiding this comment

lpinca commented Oct 14, 2017

mgjm commented Oct 14, 2017

lpinca commented Oct 14, 2017

lpinca commented Oct 14, 2017

thefourtheye Oct 15, 2017

Choose a reason for hiding this comment

BridgeAR Oct 18, 2017

Choose a reason for hiding this comment

mgjm Oct 18, 2017

Choose a reason for hiding this comment

bengl commented Oct 18, 2017

BridgeAR commented Oct 19, 2017

MylesBorins commented Oct 23, 2017

MylesBorins commented Nov 16, 2017

codebytere commented Jun 6, 2018

mgjm commented Oct 6, 2017 •

edited

Loading