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: align doc/api/tls.markdown with style guide and fix minor grammatical issues #5706

Closed
wants to merge 1 commit into from

Conversation

svozza
Copy link
Contributor

@svozza svozza commented Mar 14, 2016

Affected core subsystem(s)

Doc, TLS

Description of change

As discussed in #5677 (and tangentially, nodejs/docs#87) I've broken out the extra changes originally made in that PR into a separate one. Brings tls.markdown into alignment with the Node.js styleguide, specifically regarding the use of personal pronouns. Also, fixes various typos, punctuation errors, line wrapping violations, missing definite/indefinite articles and other minor grammatical issues.

@claudiorodriguez claudiorodriguez added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Mar 15, 2016
@svozza svozza force-pushed the doc-tls-style-guide branch 5 times, most recently from 5e04f4b to 1b2aab8 Compare March 15, 2016 09:48
@@ -352,16 +356,16 @@ gets high.
This is a wrapped version of [`net.Socket`][] that does transparent encryption
of written data and all required TLS negotiation.

This instance implements a duplex [Stream][] interfaces. It has all the
This instance implements the duplex [Stream][] interfaces. It has all the
Copy link
Member

Choose a reason for hiding this comment

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

Nit: wouldn't it be more correct to say it implements the duplex stream interface (singular)?

@benjamingr
Copy link
Member

Very nice contribution - I left some comments - when those are addressed LGTM.

@svozza
Copy link
Contributor Author

svozza commented Mar 15, 2016

Cool. I've addressed all the comments bar the two I need clarification on. I'll push the changes together when they're all done.

@svozza svozza force-pushed the doc-tls-style-guide branch 2 times, most recently from d6c2422 to e87183c Compare March 15, 2016 12:49
@svozza
Copy link
Contributor Author

svozza commented Mar 15, 2016

So I think I've addressed all the comments.

@benjamingr
Copy link
Member

Great, LGTM.

Allow me to clarify the process - pull requests are open for 48 hours during week days in order to give collaborators opportunity to respond to them and suggest changes. In a day and a half assuming no one has objections (and hopefully more people read this) this will be landed in master.

Pinging @nodejs/documentation in order to draw more attention to this issue since it's a big copy change.

@svozza
Copy link
Contributor Author

svozza commented Mar 15, 2016

Good stuff. :)

@svozza svozza force-pushed the doc-tls-style-guide branch 3 times, most recently from 3a9d37a to cb8297f Compare March 15, 2016 13:57
Brings tls.markdown into alignment with the node.js
styleguide, specifically regarding the use of
personal pronouns. Also, fixes various typos,
punctuation errors, missing definite/indefinite
articles and other minor grammatical issues.
@svozza svozza force-pushed the doc-tls-style-guide branch from cb8297f to f246ec3 Compare March 15, 2016 14:01
@svozza
Copy link
Contributor Author

svozza commented Mar 15, 2016

Actually, something I noticed is that none of the anchor links in the file work when I view the markdown files in the browser. Take, for example, this one which is supposed to point to TLS.createServer():

Actual:   #tls_tls_createserver_options_secureconnectionlistener
Expected: #tlscreateserveroptions-secureconnectionlistener

@jasnell
Copy link
Member

jasnell commented Mar 16, 2016

LGTM

benjamingr pushed a commit that referenced this pull request Mar 20, 2016
Brings tls.markdown into alignment with the node.js
styleguide, specifically regarding the use of
personal pronouns. Also, fixes various typos,
punctuation errors, missing definite/indefinite
articles and other minor grammatical issues.

PR-URL: #5706
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@benjamingr
Copy link
Member

Landed in 4f6ad5c 🎈🎈🎈

Thanks again.

@benjamingr benjamingr closed this Mar 20, 2016
@svozza
Copy link
Contributor Author

svozza commented Mar 20, 2016

👍

Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
Brings tls.markdown into alignment with the node.js
styleguide, specifically regarding the use of
personal pronouns. Also, fixes various typos,
punctuation errors, missing definite/indefinite
articles and other minor grammatical issues.

PR-URL: #5706
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Conflicts:
	doc/api/tls.markdown
@MylesBorins
Copy link
Contributor

4f6ad5c is not landing cleanly onto v4.x-staging. Would someone be willing to backport?

@svozza
Copy link
Contributor Author

svozza commented Mar 31, 2016

I can try, what exactly do I have to do? Just checkout the v4.x-staging branch and raise a PR against that?

@MylesBorins
Copy link
Contributor

@svozza check out v4.x-staging. then git cherry-pick 4f6ad5c. then work out the conflicts and send a PR to v4.x-staging

@svozza
Copy link
Contributor Author

svozza commented Mar 31, 2016

Good stuff. I'll give that a go.

svozza added a commit to svozza/node that referenced this pull request Apr 11, 2016
Brings tls.markdown into alignment with the node.js
styleguide, specifically regarding the use of
personal pronouns. Also, fixes various typos,
punctuation errors, missing definite/indefinite
articles and other minor grammatical issues.

PR-URL: nodejs#5706
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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