Using putout linter on xterm codebase by coderaiser · Pull Request #2953 · xtermjs/xterm.js

coderaiser · 2020-05-31T10:10:59Z

Just read about switching eslint rules to warnings, and want to suggest usage results of putout linter that fixes everything it can find.

Here is list of things in code of xterm.js that can be improved:

convert-for-each-to-for-of (enabled)
convert-for-to-for-of
remove-console
simplify-ternary (enabled)
remove-empty
apply-shorthand-properties (enabled)
extract-sequence-expressions (enabled)
convert-math-pow
convert-template-to-strings

Here is used config.putout.json:

{
    "rules": {
        "remove-console": "off",
        "remove-unused-variables": "off",
        "remove-useless-variables": "off",
        "apply-destructuring": "off",
        "merge-if-statements": "off",
        "convert-apply-to-spread": "off",
        "convert-math-pow": "off",
        "convert-for-to-for-of": "off",
        "convert-template-to-string": "off",
        "remove-empty": "off"
    }
}

I'm willing to run putout again disabling rules you choose If it's appropriate , I think this can make codebase of xterm.js a little bit better :).

About an error:

1) CoreMouseService
       triggerMouseEvent
         eventCodes with modifiers (DEFAULT encoding):

      AssertionError: expected [ Array(8) ] to deeply equal [ Array(4) ]
      + expected - actual

       [
      -  "\u001b[M !!"
      -  "\u001b[M!!!"
      -  "\u001b[M\"!!"
      -  "\u001b[Ma!!"
         "\u001b[M#!!"
         "\u001b[M#!!"
         "\u001b[M#!!"
         "\u001b[M`!!"
      
      at Context.<anonymous> (out/common/services/CoreMouseService.test.js:166:27)

I have the same error on master. Have no idea what it is :).

jerch

@coderaiser Did you actually look at the replacements? There are several issues with it, see my comments below.

src/common/services/CoreMouseService.test.ts

src/common/services/CoreMouseService.ts

.putout.json

src/browser/AccessibilityManager.ts

src/browser/Color.ts

src/browser/Linkifier2.ts

src/browser/renderer/Renderer.ts

src/browser/renderer/atlas/DynamicCharAtlas.ts

src/common/input/TextDecoder.ts

coderaiser · 2020-05-31T19:35:52Z

@jerch thank you for review :), I did look at replacement but want to receive feedback before proceeding. I’ll fix all comments when get to a computer.

coderaiser · 2020-05-31T20:43:34Z

@jerch just force pushed updates, we have only 4 rules not disabled, from found list :):

convert-for-each-to-for-of
simplify-ternary
apply-shorthand-properties
extract-sequence-expressions

extract-sequence-expressions find redundant code in src/common/input/Keyboard.test.ts

Also remove-unused-variables can help to find all cases similar to one find in src/browser/Terminal.ts. I can proceed with this in next PR if there is a need on it :).

jerch · 2020-06-01T12:52:10Z

@coderaiser
Did a quick microbenchmark comparing for-of with classical loop types known to be fast. Summary: for-of takes ~2 times longer on recent chrome, ~10 times longer on recent Firefox. Perfwise for-of is still smelly and does not look good compared to index loops.

I think for time critical parts we should resort to good old index-for-loops. For non critical paths imho for-of is good to go with it. forEach is way slower than all of them above, and has weird restrictions on the used type. for-of is clearly the better choice here.

@Tyriar Do you see any issues with for-of, like ECMA version supported? Imho it got official with ES6, I am not sure if TS will transpile it for lower targets correctly.

Tyriar

Pretty much agree with all your thoughts @jerch. Also I don't think we would want to include an additional linter, I'm trying to slim down out dev deps where possible.

The forEach change seemed negative to me at first, but having it not be inside a callback would help TS understand what's going on, specifically in this case which is just annoying to deal with:

I think for time critical parts we should resort to good old index-for-loops. For non critical paths imho for-of is good to go with it.

I have similar thoughts, I wouldn't even bother trying to convert for loops as we typically only use them not for perf critical parts and when we're working with the index.

Do you see any issues with for-of, like ECMA version supported? Imho it got official with ES6, I am not sure if TS will transpile it for lower targets correctly.

It transpiles down to this for es5 targets:

var a = [1, 2, 3];
for (var _i = 0, a_1 = a; _i < a_1.length; _i++) {
    var b = a_1[_i];
    console.log(b);
}

This ends up being ~40% slower in Chromium for me which isn't too bad.

remove-empty is the only other rule that I think we may want to run. If that only catches the while (reports.pop()) { }, which would definitely be better written as reports = [] as suggested anyway.

src/common/buffer/Buffer.ts

Co-authored-by: Daniel Imms <[email protected]>

coderaiser · 2020-06-01T13:33:52Z

@jerch

Did a quick microbenchmark comparing for-of with classical loop types known to be fast. Summary: for-of takes ~2 times longer on recent chrome, ~10 times longer on recent Firefox. Perfwise for-of is still smelly and does not look good compared to index loops.

I think for time critical parts we should resort to good old index-for-loops.

Absolutely agree with you :). The rule convert-for-to-for-of already disabled.

I think for time critical parts we should resort to good old index-for-loops. For non critical paths imho for-of is good to go with it. forEach is way slower than all of them above, and has weird restrictions on the used type. for-of is clearly the better choice here.

Yep, for-of definitely better then forEach :).

remove-empty is the only other rule that I think we may want to run. If that only catches the while (reports.pop()) { }, which would definitely be better written as reports = [] as suggested anyway.

Yes, I'll add it, it is have sense :).

Also I don't think we would want to include an additional linter, I'm trying to slim down out dev deps where possible.

@Tyriar I don't suggest to add an additional linter :). I suggest to add results of it work :).

jerch

LGTM 👍

coderaiser changed the title ~~lint: simplify rules using putout~~ Using putout linter on xterm codebase May 31, 2020

jerch reviewed May 31, 2020

View reviewed changes

chore: lint using putout

f63f1f7

Tyriar approved these changes Jun 1, 2020

View reviewed changes

src/common/buffer/Buffer.ts Outdated Show resolved Hide resolved

Tyriar added this to the 4.7.0 milestone Jun 1, 2020

Tyriar requested a review from jerch June 1, 2020 13:11

Update src/common/buffer/Buffer.ts

a9862a1

Co-authored-by: Daniel Imms <[email protected]>

coderaiser and others added 2 commits June 1, 2020 17:07

CoreMouseService: reports: while pop -> []

04f2b6e

Merge branch 'master' into lint/simplify

b922608

jerch approved these changes Jun 2, 2020

View reviewed changes

Tyriar merged commit 106d74c into xtermjs:master Jun 2, 2020

coderaiser deleted the lint/simplify branch June 2, 2020 14:11

coderaiser mentioned this pull request Oct 19, 2020

Using putout linter on xterm codebase part 2 #3133

Merged

coderaiser mentioned this pull request Mar 28, 2021

Lint using putout part 3 #3269

Merged

coderaiser mentioned this pull request May 14, 2021

chore: lint using putout #3341

Merged

coderaiser mentioned this pull request Nov 2, 2021

Halloween Lint using Putout (part 5) #3538

Merged

coderaiser mentioned this pull request Mar 8, 2022

Lint using 🐊Putout: part 6 #3674

Merged

coderaiser mentioned this pull request Sep 14, 2022

Lint using 🐊Putout: part 7 #4118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using putout linter on xterm codebase#2953

Using putout linter on xterm codebase#2953
Tyriar merged 4 commits intoxtermjs:masterfrom
coderaiser:lint/simplify

coderaiser commented May 31, 2020 •

edited

Loading

Uh oh!

jerch left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderaiser commented May 31, 2020

Uh oh!

coderaiser commented May 31, 2020 •

edited

Loading

Uh oh!

jerch commented Jun 1, 2020

Uh oh!

Tyriar left a comment

Uh oh!

Uh oh!

coderaiser commented Jun 1, 2020 •

edited

Loading

Uh oh!

jerch left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

coderaiser commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderaiser commented May 31, 2020

Uh oh!

coderaiser commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerch commented Jun 1, 2020

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderaiser commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderaiser commented May 31, 2020 •

edited

Loading

jerch left a comment •

edited

Loading

coderaiser commented May 31, 2020 •

edited

Loading

coderaiser commented Jun 1, 2020 •

edited

Loading