benchmark,lib: var to const by BridgeAR · Pull Request #26915 · nodejs/node (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This came up recently again. I just ran a modified eslint rule against lib and benchmark and I have no strong opinion about actually doing this and I am absolutely fine with closing this again. It's just there to resolve a comment in the referenced PR.

Note: I did not update any eslint rules as there is no explicit rule to do this. Instead, I just modified the prefer-const rule to match against var instead of let.

Refs: #26679

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Choose a reason for hiding this comment

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

I know we have shot these PRs down multiple times, but this one is only 343 lines and if that's all it takes to make all possible var → const replacements, I think we should do it.

BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 26, 2019

Note: I did not update any eslint rules as there is no explicit rule to do this. Instead, I just modified the prefer-const rule to match against var instead of let.

const and let have the same scoping rules so that ESLint rule makes sense. But var is different. Is there a chance that this introduces subtle bugs where some code expects a variable to be hoisted but now it isn't? I'm guessing there isn't a problem because all the tests are passing, but an extra careful eye in review and maybe even a CITGM run might be worthwhile. (Not saying it's semver major--just saying extra test exercising via CITGM might be a good extra-cautious measure.)

@Trott eslint itself should warn in case the scope is not correct anymore. There could in theory also be changes that can not be auto detected but that's unlikely. Especially with our test suite.

@Trott eslint itself should warn in case the scope is not correct anymore.

Ah, yes, of course.

There could in theory also be changes that can not be auto detected but that's unlikely. Especially with our test suite.

👍

Rebased due to conflicts.

targos pushed a commit that referenced this pull request

Mar 30, 2019

Refs: #26679

PR-URL: #26915 Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Tobias Nießen tniessen@tnie.de Reviewed-By: Refael Ackermann refack@gmail.com

BethGriggs pushed a commit that referenced this pull request

Apr 5, 2019

Refs: #26679

PR-URL: #26915 Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Tobias Nießen tniessen@tnie.de Reviewed-By: Refael Ackermann refack@gmail.com

BethGriggs pushed a commit that referenced this pull request

Apr 8, 2019

Refs: #26679

PR-URL: #26915 Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Tobias Nießen tniessen@tnie.de Reviewed-By: Refael Ackermann refack@gmail.com Signed-off-by: Beth Griggs Bethany.Griggs@uk.ibm.com

Reviewers

refack refack approved these changes