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.

Sign up for GitHub

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

Jump to bottom

doc: improve grammar in tls docs regarding #4246 #4315

Closed
wants to merge 1 commit into from
Closed

doc: improve grammar in tls docs regarding #4246 #4315

wants to merge 1 commit into from

Conversation

AdriVanHoudt
Copy link
Contributor

AdriVanHoudt referenced this pull request Dec 16, 2015
@indutny
tls: introduce secureContext for tls.connect
c5b4f6b 
Add `secureContext` option to `tls.connect`. It is useful for caching
client certificates, key, and CA certificates.

PR-URL: #4246
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex
Copy link
Contributor

mscdex commented Dec 16, 2015

IMHO it would be better to have a more descriptive commit message like "doc: improve consistency and grammar in tls docs" instead of just linking to a PR.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Dec 16, 2015
@AdriVanHoudt
Copy link
Contributor Author

Ow yeah first pr so. I can update if you want.

@jasnell
Copy link
Member

jasnell commented Dec 16, 2015

LGTM with the commit log change

indutny
indutny reviewed Dec 16, 2015
View reviewed changes
doc/api/tls.markdown
@@ -542,9 +542,9 @@ Creates a new client connection to the given `port` and `host` (old API) or
`options.port` and `options.host`. (If `host` is omitted, it defaults to
`localhost`.) `options` should be an object which specifies:

- `host`: Host the client should connect to
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this PR neat, please consider submitting this fixes in a separate pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np

@AdriVanHoudt
Copy link
Contributor Author

Updated the commit log or do you not want to reference the PR at all?

@AdriVanHoudt
Copy link
Contributor Author

Updated the commit per @indutny's comment

@AdriVanHoudt AdriVanHoudt changed the title doc: fix wording of #4246 doc: improve grammar in tls docs regarding #4246 Dec 16, 2015
@indutny
Copy link
Member

indutny commented Dec 16, 2015

@AdriVanHoudt I know it is nitpicking, but could I ask you to move #4246 in the tilte to Fix: #4246 in the commit description.

@indutny
Copy link
Member

indutny commented Dec 16, 2015

Otherwise LGTM, thank you!

@AdriVanHoudt
Copy link
Contributor Author

@indutny you want me to edit the title to remove the #4246 mention?

@AdriVanHoudt
Copy link
Contributor Author

And also not a problem I understand you want a clean log of everything

@indutny
Copy link
Member

indutny commented Dec 17, 2015

@AdriVanHoudt almost there! Could you please move Fix: ... to commit log instead of title?

@AdriVanHoudt
Copy link
Contributor Author

@indutny uh the commit says that. Not sure what you mean.

@indutny
Copy link
Member

indutny commented Dec 17, 2015

@AdriVanHoudt the commit has everything on the same line: AdriVanHoudt@54ff778

Please take a look at: 2a60e2a to see how it could be improved. Also https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit may be useful.

@AdriVanHoudt
Copy link
Contributor Author

@indutny ah I see now. Do I also need to put the PR url in there?

@indutny
Copy link
Member

indutny commented Dec 17, 2015

@AdriVanHoudt that and reviewed-by will be put by us ;) But thanks for asking!

@AdriVanHoudt
Copy link
Contributor Author

@indutny ok fixed it

jasnell pushed a commit that referenced this pull request Dec 24, 2015
@AdriVanHoudt @jasnell
doc: improve grammar in tls docs
2fd3d8a 
Fix: #4246
PR-URL: #4315
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@jasnell
Copy link
Member

jasnell commented Dec 24, 2015

Landed in 2fd3d8a

@jasnell jasnell closed this Dec 24, 2015
@AdriVanHoudt
Copy link
Contributor Author

👍

@MylesBorins
Copy link
Contributor

It looks like the commit this modifies is semver minor and not in LTS. Removing lts-watch, please readd if I am mistaken @jasnell

@rvagg
Copy link
Member

rvagg commented Jan 4, 2016

Hey @AdriVanHoudt, I think this is your first commit to core, congrats! Even though it's only a trivial change we appreciate the attention to detail and hope you stick around and continue to contribute!

@AdriVanHoudt
Copy link
Contributor Author

@rvagg thanks! Will definitely do another pr to fix those . missing at the end of the first to items in the list :P more complex stuff is beyond my skill (for now) but will definitely keep reading the PR's in the changelog!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@AdriVanHoudt @Fishrock123
doc: improve grammar in tls docs
6450d86 
Fix: nodejs#4246
PR-URL: nodejs#4315
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@MylesBorins MylesBorins mentioned this pull request Jan 14, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@AdriVanHoudt
doc: improve grammar in tls docs
fb56a3a 
Fix: nodejs#4246
PR-URL: nodejs#4315
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AdriVanHoudt @mscdex @jasnell @indutny @MylesBorins @rvagg