WIP: Introduce eslint, and conform to its recommended rule set #197
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
With the "recommended" eslint set
I ignored any weird or inconsistent indentation. Just killing tabs.
Eslint flagged this unused variable. Seems like a typo to me.
I'm guessing this was a typo, since the previous opcode in the file is 107 SetScreenSaver, and the wrong opcode for this was 108.
Lots of unused arguments are around, but I would feel bad deleting them because they make the code easier to read. So let's just prepend them with an underscore to make it clear not using them was intentional.
- Missing initialisers. - Switched `var` for `let` where they shadow another variable. (A full `var` → `const`/`let` pass would be good later.)
Just an unused variable.
- Deleted some Unused arguments. - Fixed some redeclared variables, either by making them `let` or adding blocks to give variables their own scope. I'm not sure why :323 is a bug or leftovers, so I left it erroring for now.
Mostly a bunch of `let`s to avoid redefining stuff that's technically in the same scope. I don't know what the purpose of the `stash` function is, so I left it alone. It isn't called anywhere. Is it for debugging like hexy?
unused require
- Unused require - Underscore-prefixing unused arguments
Dead code mostly
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.
We talked about maybe doing this in 2018, and for some reason I woke up today wanting to eslint something. 🤔 Maybe it's because I've been playing Factorio for 2 days straight.
So I fired it up with
"eslint:recommended"-settings and beat the red text with hammers until I got tired. Here's the progress so far.I've fixed stuff that looked obvious (unused variables, mixed tabbing,
vars overriding each other, …) but left eslint erroring if I felt unsure. I mostly did only the minimum to resolve the eslint error, so for example while a fullvar→const/letreplacement round would be a good idea, I've so far just been replacing them individually where they fix eslint errors.Ones I'm unsure about that @sidorares should maybe check that they're no problem:
lib/unpackstream.js:323→ takes an unusedstreamargument, but thestream.writecall is commented out; is that a bug, or a remnant from an old implementation?lib/xcore.js:22→stashisn't called anywhere, is it for debugging likehexy? Seems like a no-op?lib/ext/render.js:34→ thenum_*stuff is extracted but not used. Should they be inres?lib/ext/render.js:99→ Unused function. For debugging?lib/ext/render.js:435→ Unused variableglyphPaddedLength. Sounds meaningful but isn't used. Is it a logical error?lib/ext/render.js:647→ Unused variablecharFormat. Again sounds meaningful. Check logic.lib/ext/render.js:650→ as just above ↑lib/ext/apple-wm.js:186→SetWindowMenuonly contains dead code, so I commented it out. Probably in-progress and intentional?Marked items either passed lint already, or I did a basic pass over them (adding notable issues to the above list for competent review):