Skip to content

Update tls_wrap.cc #2486

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
Closed
wants to merge 3 commits into from
Closed

Update tls_wrap.cc #2486

wants to merge 3 commits into from

Conversation

@JungMinu
Copy link
Member

@JungMinu JungMinu commented Aug 21, 2015

fix grammatically wrong expression

@JungMinu
Update tls_wrap.cc
27b7869 
fix grammatically wrong expression
@mscdex mscdex added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 21, 2015
@silverwind silverwind added doc Issues and PRs related to the documentations. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 21, 2015
brendanashworth
brendanashworth reviewed Aug 21, 2015
View reviewed changes
src/tls_wrap.cc Outdated
Copy link
Contributor

@brendanashworth brendanashworth Aug 21, 2015

Choose a reason for hiding this comment

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

this is an improvement but there are still two issues in the comment here (ex 'socket' -> 'the socket', 'callback' -> 'the callback'), would you mind fixing the rest while you're here? :)

Copy link
Member Author

@JungMinu JungMinu Aug 22, 2015

Choose a reason for hiding this comment

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

@brendanashworth Thanks! I fixed the rest :)

@JungMinu
Update tls_wrap.cc
bd70d4c 
fix grammatically wrong expression
@JungMinu
Merge pull request #2 from codevelop/codevelop-patch-2
e634bc6 
Update tls_wrap.cc
@JungMinu
Copy link
Member Author

JungMinu commented Aug 21, 2015

@brendanashworth Thanks for the comment!
I fixed the rest!

@jasnell
Copy link
Member

jasnell commented Aug 22, 2015

LGTM

@JungMinu
Copy link
Member Author

JungMinu commented Aug 22, 2015

@jasnell Thanks!

@jasnell
Copy link
Member

jasnell commented Aug 22, 2015

Can I ask you to please squash the commits down into a single and update the commit log to the pattern shown here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit. Once that's done I'll queue it up to land.

@JungMinu JungMinu closed this Aug 22, 2015
@JungMinu JungMinu reopened this Aug 22, 2015
@JungMinu
Copy link
Member Author

JungMinu commented Aug 22, 2015

@jasnell I remade a new pull request into a single commit and updated the commit log:
#2490

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

@JungMinu @jasnell @brendanashworth @mscdex @silverwind @Fishrock123