doc: fix inconsistent documentation of server.listen() by mgjm · Pull Request #16020 · nodejs/node · GitHub

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.

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

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`][].

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.

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

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?

@mgjm no this is perfect.

@bengl @sam-github @joyeecheung PTAL.

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

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 <bryan@bryanenglish.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Landed in 278f653

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

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 <bryan@bryanenglish.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <bryan@bryanenglish.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <bryan@bryanenglish.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <bryan@bryanenglish.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

-              *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`][].

               [`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

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

doc: fix inconsistent documentation of server.listen() by mgjm · Pull Request #16020 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

doc: fix inconsistent documentation of server.listen()#16020mgjm wants to merge 4 commits intonodejs:masternodejs/node:masterfrom mgjm-fork:fix-documentaion-listenmgjm-fork/node:fix-documentaion-listenCopy head branch name to clipboard

Conversation

mgjm commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Oct 10, 2017

Uh oh!

joyeecheung Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

mgjm Oct 14, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

mgjm Oct 14, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca commented Oct 14, 2017

Uh oh!

mgjm commented Oct 14, 2017

Uh oh!

lpinca commented Oct 14, 2017

Uh oh!

lpinca commented Oct 14, 2017

Uh oh!

thefourtheye Oct 15, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

mgjm Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

bengl commented Oct 18, 2017

Uh oh!

BridgeAR commented Oct 19, 2017

Uh oh!

MylesBorins commented Oct 23, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

codebytere commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

doc: fix inconsistent documentation of server.listen()#16020
mgjm wants to merge 4 commits into
nodejs:masterfrom
mgjm-fork:fix-documentaion-listen

mgjm commented Oct 6, 2017 •
edited
Loading