Fix 11 lints in src/plugins/tabletools.js #1039
Merged
Conversation
This file contains 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
``` src/plugins/tabletools.js 53:5 error Unexpected var, use let or const instead no-var 167:4 error Unexpected var, use let or const instead no-var 196:4 error Unexpected var, use let or const instead no-var 212:9 error Unexpected var, use let or const instead no-var 320:8 error Unexpected var, use let or const instead no-var 329:4 error Unexpected var, use let or const instead no-var 370:8 error Unexpected var, use let or const instead no-var 371:9 error Unexpected var, use let or const instead no-var 591:8 error Unexpected var, use let or const instead no-var 709:4 error Unexpected var, use let or const instead no-var 803:8 error 'lang' is assigned a value but never used no-unused-vars ``` `lang` is safe to remove because the last references to it look like they were removed in c2d3248 ("Remove ui-related code from plugins", 2015). The others I changed to `let` or `const` as appropriate. This code leans too heavily on `let` semantics in general, because values are reassigned and accessed sometimes quite large distances away. One particularly nasty case was a `for (var i = 0, rows = ...)`. This `rows` var is initialized in the loop and then accessed quite a bit further down outside the loop. For that one I had to hoist the `let rows` higher up in the scope. This is a bit of a foot-gun, and I only caught it by testing. Test plan: `npm run dev && npm run test && npm run start` and test demo (specifically tables). Related: #990
wincent
commented
Jan 30, 2019
| for (var i = 0, rows = map.length; i < rows; i++) { | ||
| for (var j = 0, cols = map[i].length; j < cols; j++) { | ||
| for (let i = 0, rows = map.length; i < rows; i++) { | ||
| for (let j = 0, cols = map[i].length; j < cols; j++) { |
There was a problem hiding this comment.
Note this cols doesn't suffer the same problem as rows in the line above. cols is only accessed in the loop condition and not outside of it.
|
Just started reviewing :) |
Sent from 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
langis safe to remove because the last references to it look like they were removed in c2d3248 ("Remove ui-related code from plugins", 2015).The others I changed to
letorconstas appropriate. This code leans too heavily onletsemantics in general, because values are reassigned and accessed sometimes quite large distances away.One particularly nasty case was a
for (var i = 0, rows = ...). Thisrowsvar is initialized in the loop and then accessed quite a bit further down outside the loop. For that one I had to hoist thelet rowshigher up in the scope. This is a bit of a foot-gun, and I only caught it by testing.Test plan:
npm run dev && npm run test && npm run startand test demo(specifically tables).
Related: #990