-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
clear out const/let #9878
clear out const/let #9878
Conversation
@stpCollabr8nLstn May I kindly ask you to format the commit message as described in CONTRIBUTING guidelines. |
31f9f33
to
897b073
Compare
@imyller fixed |
@stpCollabr8nLstn Thank you. Looks good 👍 |
var common = require('../common'); | ||
var assert = require('assert'); | ||
const common = require('../common'); | ||
const assert = require('assert'); |
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.
Nit: I think it's ok to move the assert
require after the common.hasCrypto
check.
I'm pointing this out because other tests do this and it would be nice for consistency.
Thanks.
@stpCollabr8nLstn The commit author details have your name as |
@gibfahn is there a way to retroactively change that? |
git commit --no-edit --amend --author="Jane Q. Public <email@example.com>"
git push --force-with-lease origin notMaster To change it globally so it will be the default for future commits: git config --global user.name "Jane Q. Public" |
b85439e
to
c6423ce
Compare
thanks @Trott done |
clear out const/let change assert.notEqual to assert move assert after common.hasCrypto check
Even though the code shouldn't have changed, it's probably a good practice to always re-run CI after a force push, so.... |
Landed in efa8456. |
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. Thank you for the PR and for participating in Code and Learn! Welcome to Node.js :-)
clear out const/let change assert.notEqual to assert move assert after common.hasCrypto check PR-URL: #9878 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
clear out const/let change assert.notEqual to assert move assert after common.hasCrypto check PR-URL: #9878 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
clear out const/let change assert.notEqual to assert move assert after common.hasCrypto check PR-URL: nodejs#9878 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
clear out const/let change assert.notEqual to assert move assert after common.hasCrypto check PR-URL: nodejs#9878 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
clear out const/let change assert.notEqual to assert move assert after common.hasCrypto check PR-URL: #9878 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
change let to const/let
change assert.notEqual to assert