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: add note about tls connection meta data methods #3746

Closed
wants to merge 1 commit into from

Conversation

DaftMonk
Copy link
Contributor

Document current behavior of TLSSocket, as discussed in #3545

@r-52 r-52 added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Nov 10, 2015
@mscdex
Copy link
Contributor

mscdex commented Nov 10, 2015

When I first read this, I didn't know what methods were being referred to. You might include an example method (e.g. getPeerCertificate()) or two.

@DaftMonk
Copy link
Contributor Author

@mscdex Added an example.

@@ -764,6 +764,8 @@ of written data and all required TLS negotiation.
This instance implements a duplex [Stream][] interfaces. It has all the
common stream methods and events.

Methods that return TLS connection meta data (e.g. `getPeerCertificate()`) will only return data while the connection is open.
Copy link
Member

Choose a reason for hiding this comment

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

Please wrap at 80 columns. Maybe you can turn getPeerCertificate() into a link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use correct column length.

I couldn't get intra-page links working for that section. I tried using the markdown

[getPeerCertificate](#tlssocketgetpeercertificate-detailed-)

and when I opened the preview in github and clicked on it, it didn't work. However typing in the url directly https://github.com/DaftMonk/node/blob/docs-tls-socket/doc/api/tls.markdown#tlssocketgetpeercertificate-detailed- did work.

So I'm not sure what the issue was, but I reverted it back to a code example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, I tried again and the link worked this time.

@DaftMonk DaftMonk force-pushed the docs-tls-socket branch 5 times, most recently from 1f04c0b to 0a414e8 Compare November 10, 2015 21:24
@bnoordhuis
Copy link
Member

Thanks, landed in eff8c3e.

@bnoordhuis bnoordhuis closed this Nov 10, 2015
bnoordhuis pushed a commit that referenced this pull request Nov 10, 2015
PR-URL: #3746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this pull request Nov 11, 2015
PR-URL: #3746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
MylesBorins pushed a commit that referenced this pull request Nov 17, 2015
PR-URL: #3746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

landed in v4.x-staging in 8b6120d

rvagg pushed a commit that referenced this pull request Dec 4, 2015
PR-URL: #3746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
PR-URL: #3746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
PR-URL: #3746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants