-
Notifications
You must be signed in to change notification settings - Fork 30k
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
test: change var to const and add mustCall in crypto tests #9954
test: change var to const and add mustCall in crypto tests #9954
Conversation
First line of commit is too long, see the commit message guidelines here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message is too long. keep it short as per the guideline!
|
||
if (!common.hasCrypto) { | ||
common.skip('missing crypto'); | ||
process.exit(); | ||
} | ||
var tls = require('tls'); | ||
const tls = require('tls'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an extra space here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with formatting nits addressed.
Ping @harishtejwani: Can you remove the extra space identified by @cjihrig? |
Sure, sorry will do later this week
…Sent from my iPhone
On Dec 21, 2016, at 4:15 PM, Rich Trott ***@***.***> wrote:
Ping @harishtejwani: Can you remove the extra space identified by @cjihrig?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
2c7e030
to
d425490
Compare
@harishtejwani Because it was such a small change (remove one space character), I went ahead and did it for you and force pushed to your branch. @cjihrig Can you update your review if appropriate? Thanks! |
change var to const and add mustCall PR-URL: nodejs#9954 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in 0ff69b4. |
change var to const and add mustCall PR-URL: #9954 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
change var to const and add mustCall PR-URL: #9954 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
change var to const and add mustCall PR-URL: #9954 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
Description of change
test: change var to const and add mustCall in crypto tests. Did not add mustCall on process exit