doc: Socket.prototype.connect(options[, connectListener]) #951
Conversation
|
LGTM nice addition. |
|
This is basically duplication the documentation for those options. Maybe we can make the documentation so it does not need duplication. Also, do we want to document this method or should it be considered internal? |
|
It's not an internal function. I own the I'm going to add another commit to change the documentation for net.connect() to explain that it's a convenience function. That should remove some of the duplicacy. I can also specify that the other Socket.connect that takes specific non-option arguments just do what the object taking version does, but with only the arguments passed. I'm not sure whether I should do this or not though. Suggestions? |
Havvy
changed the title
| A factory method, which returns a new ['net.Socket'](#net_class_net_socket) | ||
| and connects to the supplied address and port. | ||
| A factory function, which returns a new ['net.Socket'](#net_class_net_socket) | ||
| and automatically connects with the supplied. |
There was a problem hiding this comment.
I'm not a native english speaker, but after you remove address and port this doesn't make much sense to me anymore.
|
Both of those were typos, and have been fixed. Thanks for catching. |
| ## net.connect(path[, connectListener]) | ||
| ## net.createConnection(path[, connectListener]) | ||
|
|
||
| Creates unix socket connection to `path`. | ||
| A factory function, which returns a new unix ['net.Socket'](#net_class_net_socket) | ||
| and automatically connects to the supplied path. |
There was a problem hiding this comment.
Maybe adding backticks to path?
|
Backticks added. I also decided to keep the host defaults to localhost note for the port & host variant of the function. |
| @@ -3,7 +3,7 @@ | |||
| Stability: 3 - Stable | |||
|
|
|||
| The `net` module provides you with an asynchronous network wrapper. It contains | |||
| methods for creating both servers and clients (called streams). You can include | |||
| functions for creating both servers and clients (called streams). You can include | |||
| this module with `require('net');` | |||
There was a problem hiding this comment.
Do we want a period at the end here?
|
Other than squashing and minor question lgtm. |
|
I didn't take out the period, but it seems like a good idea to have a period there. Added in, and squashed. |
|
+1, nicely written, well refactored to avoid doc duplication. I appreciate the improvements to punctuation and grammar. |
| a FIN packet when the other end of the socket sends a FIN packet. | ||
| Defaults to `false`. See ['end'][] event for more information. | ||
| The options are passed to both the ['net.Socket'](#net_class_net_socket) | ||
| constructor and the ['Socket.connect'](#net_socket_connect_options_connectlistener) |
There was a problem hiding this comment.
Shouldn't the object name be in lowercase? (socket.connect)
Yes. Fixed. |
| A factory function, which returns a new ['net.Socket'](#net_class_net_socket) | ||
| and automatically connects to the supplied `port` and `host`. | ||
|
|
||
| If host is omitted, 'localhost' will be assumed. |
|
Can you please rebase against the latest v1.x branch? |
|
I added backticks to I rebased against the latest v1.x branch. Should I add backticks to the links or not? Other links I've seen have not had them. |
| ## net.connect(path[, connectListener]) | ||
| ## net.createConnection(path[, connectListener]) | ||
|
|
||
| Creates unix socket connection to `path`. | ||
| A factory function, which returns a new unix ['net.Socket'](#net_class_net_socket) |
There was a problem hiding this comment.
Should I move this link into the next link? The line is 83 characters long.
There was a problem hiding this comment.
I had a similar issue in the fs.markdown docs patch - couldn't figure out a way to make it drop a line in the markdown but not in the .html
There was a problem hiding this comment.
Yes. Is not a big problem that some lines are a little short. It will be at same line in the rendered version anyway.
No, links usually doesn't have backticks. |
net.Socket::connect(options[, connectListener]) was missing, with the relevant details found on the net.connect function. I moved the appropriate documentation over and then rewrote the documentation for the function to say that it just creates a socket and calls the connect method on it. I also changed the other net.Socket::connect variants to say they are like the options version but called with a specific options object. net.connect and other functions were called methods even though they don't use the `this` binding, so I changed method to function where appropriate. Finally, I added a missing period to the end of the module summary. It's not really related to the rest of the changes, but benjamingr noticed it.
|
All current Nits fixed. |
net.Socket::connect(options[, connectListener]) was missing, with the relevant details found on the net.connect function. I moved the appropriate documentation over and then rewrote the documentation for the function to say that it just creates a socket and calls the connect method on it. I also changed the other net.Socket::connect variants to say they are like the options version but called with a specific options object. net.connect and other functions were called methods even though they don't use the `this` binding, so I changed method to function where appropriate. Finally, I added a missing period to the end of the module summary. It's not really related to the rest of the changes, but benjamingr noticed it. PR-URL: #951 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Christian Tellnes <[email protected]>
|
Merged as 6d26990 |
I just copied the relevant options information from net.createConnection and the points about being asynchronous/unneeded from the other defined connect method.
The source code claims that the socket.connect parameters that were already documented is deprecated.
I think that
net.connect(options[, connectListener])should say that it passes the options object to both the Socket constructor and the connect method, and then have all options documented (they currently are not!).