chore: lint using putout by coderaiser · Pull Request #3341 · xtermjs/xterm.js

coderaiser · 2021-05-14T10:32:01Z

After successfully merged linting sessions part1, part2 and part3, the time is come for part 4 :).

As usual, any rule can be disabled.
Command used:

putout {src,addons}/**/*.ts --fix

Applied rules:

Current config for putout v17:

{
    "rules": {
        "apply-shorthand-properties": "on",
        "apply-destructuring": "off",
        "apply-numeric-separators": "off",
        "convert-assignment-to-comparison": "off",
        "convert-apply-to-spread": "off",
        "convert-math-pow": "off",
        "convert-for-to-for-of": "off",
        "convert-template-to-string": "off",
        "strict-mode": "off",
        "remove-useless-spread/object": "off",
        "remove-useless-array-constructor": "off",
        "remove-boolean-from-assertions": "off",
        "remove-iife": "off",
        "remove-console": "off",
        "remove-unused-variables": "off",
        "remove-useless-variables": "off",
        "merge-if-statements": "off",
        "promises/add-missing-await": "off"
    },
    "match": {
        "SerializeAddon.benchmark.ts": {
            "remove-unused-expressions": "off"
        }
    },
    "plugins": [
        "apply-shorthand-properties"
    ]
}

src/common/Clone.ts

src/browser/Terminal.ts

addons/xterm-addon-serialize/src/SerializeAddon.ts

coderaiser · 2021-05-14T15:37:35Z

Just disabled rules:

And fixed:

simplify-ternary(@putout/[email protected])

And updated PR, thanks a lot for quick response @Tyriar :)!

src/common/Clone.ts

Tyriar · 2021-05-14T16:50:34Z

src/browser/TestUtils.test.ts

    return true;
  }
-  public getJoinedCharacters(row: number): [number, number][] {
+  public getJoinedCharacters(row: number): Array<[number, number]> {


We prefer [] to Array<>, I think this is an eslint rule but not sure if we use it

Looks like it's related to 8c65fca, maybe I had old master 🤔 . I'm also prefer shorthand, and there is a rule convert-generic-to-shorthand.

That was a weirdo before - we had an array rule in place, but the older eslint version did not care much about it. After upgrading I got several linting warnings, and had to change the rule in 8c65fca to pass without warnings.

jerch

Just some comments/questions from my side.

jerch · 2021-05-17T11:38:45Z

addons/xterm-addon-serialize/test/SerializeAddon.api.ts

  for (let i = 0; i < array.length; i++) {
    if (typeof initial === 'function') {
-      array[i] = (<(index: number) => T>initial)(i);
+      array[i] = (initial as (index: number) => T)(i);


@Tyriar Do we have a preference here? Could/should this be an eslint rule?

I admit, that I like the xy as Z more than <Z>xy, its imho easier to grasp with less syntax overhead (makes me less thinking). But thats just me.

The thing is Babel can’t parse code that have both react and typescript enabled, for xterm.js it’s not a thing, but new construction is preferable.

I prefer as too, more readable and a little easier to type. Looks like there is an eslint rule #3359

Yes, this is the same rule, with the only difference: it cannot fix :). Anyways it will keep codebase in a clean state warning developers about assertions type used, so I think it is useful.

Yeah I noticed that when I did the PR ☹️

jerch · 2021-05-17T11:42:20Z

addons/xterm-addon-webgl/src/GlyphRenderer.ts

        // from bg. This is needed since the inverse fg color should be based on the original bg
        // color, not on the selection color
-        fg = (fg & ~(Attributes.CM_MASK | Attributes.RGB_MASK | FgFlags.INVERSE));
+        fg &= ~(Attributes.CM_MASK | Attributes.RGB_MASK | FgFlags.INVERSE);


👍 for shorthand notation where possible.

Minor sidenote - in some older engines those were actually slower than the explicit long versions.

jerch · 2021-05-17T11:46:57Z

src/browser/Linkifier.test.ts

        linkifier.registerLinkMatcher(/test/, () => assert.fail(), {
          validationCallback: (url, cb) => {
-            count += 1;
+            ++count;


@coderaiser Is there a reason for ++count instead of count++? In C I kinda always use the latter, if I dont bind the result, which leads to same ASM code, but always use the first in loops (which used to make a difference in old compilers, not anymore though). Is there a difference in JS?

I always prefer the postfix as well, with the only exception being if I actually need the updated value

There is only one reason: you see what’s going on the variable in the first place, so you can understand it faster:

aVeryLongVariable++

Or like in vim dw - remove a word. So when you see ++ at the beginning you already know what's going on next, in other case you read all the variable and just after you see what to do.

Anyways, it’s not a problem, I’ll add an option or change to postfix and disable a rule.

Just updated

jerch · 2021-05-17T11:55:40Z

src/common/InputHandler.test.ts

      bufferService.reset();
    });
-    it('SL (scrollLeft)', async () => {
+    it('SL (scrollLeft)', () => {


Eww, guess I created a code smell here:

Your tool correctly found the asyncs to be stripped. The code smell happens at those parseP calls, those should have await in front to be consistent on code level. Created #3347 for that. (You can keep the rule as it is, gonna re-add the async keywords over there.)

Tyriar

Looks good, let's wait until #3360 is merged to merge this in since it fixes more as problems and also some typing bugs (like setTimeout instead of window.setTimeout)

src/browser/Terminal2.test.ts

Tyriar requested changes May 14, 2021

View reviewed changes

src/common/Clone.ts Outdated Show resolved Hide resolved

src/browser/Terminal.ts Outdated Show resolved Hide resolved

addons/xterm-addon-serialize/src/SerializeAddon.ts Outdated Show resolved Hide resolved

Tyriar added this to the 4.13.0 milestone May 14, 2021

Tyriar requested changes May 14, 2021

View reviewed changes

coderaiser and others added 2 commits May 15, 2021 01:17

chore: lint using putout

d046e2a

Merge branch 'master' into feature/lint-with-putout

e42e20f

jerch reviewed May 17, 2021

View reviewed changes

coderaiser and others added 3 commits May 17, 2021 17:30

Merge branch 'master' into feature/lint-with-putout

91a79e9

chore: browser: Linkifier: increment

ea7f898

Merge branch 'master' into feature/lint-with-putout

ca70d7b

Tyriar approved these changes Jun 4, 2021

View reviewed changes

src/browser/Terminal2.test.ts Outdated Show resolved Hide resolved

coderaiser and others added 2 commits June 4, 2021 19:37

chore: inc -> prefix -> postfix

6801793

Merge branch 'master' into feature/lint-with-putout

096655a

Tyriar merged commit 96806ee into xtermjs:master Jun 4, 2021

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

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderaiser May 14, 2021 • 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!

jerch May 17, 2021 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tyriar Jun 4, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderaiser May 17, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderaiser commented May 14, 2021 •

edited

Loading

coderaiser commented May 14, 2021 •

edited

Loading

coderaiser May 14, 2021 •

edited

Loading

jerch May 17, 2021 •

edited

Loading

jerch left a comment •

edited

Loading

Tyriar Jun 4, 2021 •

edited

Loading

coderaiser May 17, 2021 •

edited

Loading