-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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 notMasterTo 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