-
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
test: replace var with const in test-require-dot #9916
Conversation
closed #9896 as the commit message was not following the guidelines and git commit --amend resulted into 3 commits. |
var module = require('module'); | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const m = require('module'); |
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.
why not keeping module
as it is?
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.
keeping the module as it is causes an error, hence renaming it something else:
/Users/amarz/summit16/node/test/parallel/test-require-dot.js:4
const module = require('module');
^
SyntaxError: Identifier 'module' has already been declared
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 if CI is happy.
FYI, the extra commits possibly came from doing a git pull
.
|
||
assert.equal(c.value, 42, 'require(".") should honor NODE_PATH'); | ||
assert.strictEqual(c.value, 42, 'require(".") should honor NODE_PATH'); |
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: can you please add a final new line.
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 once the lint errors have been fixed.
Pushed a commit that added a new line char to satisfy the linter. |
Landed fa4f158 Thanks for the contribution. |
PR-URL: #9916 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>
PR-URL: #9916 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>
PR-URL: #9916 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>
PR-URL: #9916 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
Description of change
test: replace
var
withconst
in test-require-dot