Skip to content
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: Socket.prototype.connect(options[, connectListener]) #951

Closed
wants to merge 1 commit into from
Closed

doc: Socket.prototype.connect(options[, connectListener]) #951

wants to merge 1 commit into from

Conversation

Havvy
Copy link
Contributor

@Havvy Havvy commented Feb 25, 2015

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!).

@micnic micnic added the doc Issues and PRs related to the documentations. label Feb 25, 2015
@benjamingr
Copy link
Member

LGTM nice addition.

@tellnes
Copy link
Contributor

tellnes commented Feb 26, 2015

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?

@Havvy
Copy link
Contributor Author

Havvy commented Feb 27, 2015

It's not an internal function. I own the irc-socket module, which upgrades a net.Socket or tls.Socket into an IrcSocket, and I tell people to pass in an unconnected socket. IMO, the documentation before this patch is backwards in that the primitive operation is not properly documented.

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 Havvy changed the title Socket.prototype.connect(options[, connectListener]) doc: Socket.prototype.connect(options[, connectListener]) Feb 27, 2015
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a native english speaker, but after you remove address and port this doesn't make much sense to me anymore.

@Havvy
Copy link
Contributor Author

Havvy commented Feb 27, 2015

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe adding backticks to path?

@Havvy
Copy link
Contributor Author

Havvy commented Feb 27, 2015

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');`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a period at the end here?

@benjamingr
Copy link
Member

Other than squashing and minor question lgtm.

@Havvy
Copy link
Contributor Author

Havvy commented Feb 27, 2015

I didn't take out the period, but it seems like a good idea to have a period there. Added in, and squashed.

@sam-github
Copy link
Contributor

+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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the object name be in lowercase? (socket.connect)

@Havvy
Copy link
Contributor Author

Havvy commented Mar 1, 2015

Shouldn't the object name be in lowercase? (socket.connect) - tellnes

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing backticks

@tellnes
Copy link
Contributor

tellnes commented Mar 1, 2015

Can you please rebase against the latest v1.x branch?

@Havvy
Copy link
Contributor Author

Havvy commented Mar 1, 2015

I added backticks to host and 'localhost'.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I move this link into the next link? The line is 83 characters long.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Is not a big problem that some lines are a little short. It will be at same line in the rendered version anyway.

@tellnes
Copy link
Contributor

tellnes commented Mar 1, 2015

Should I add backticks to the links or not? Other links I've seen have not had them.

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.
@Havvy
Copy link
Contributor Author

Havvy commented Mar 1, 2015

All current Nits fixed.

tellnes pushed a commit that referenced this pull request Mar 1, 2015
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]>
@tellnes
Copy link
Contributor

tellnes commented Mar 1, 2015

Merged as 6d26990

@tellnes tellnes closed this Mar 1, 2015
@rvagg rvagg mentioned this pull request Mar 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants