Lint using 🐊Putout: part 7 #4118

coderaiser · 2022-09-14T23:21:25Z

Throughout your life advance daily, becoming more skillful than yesterday more skillful than today. This is never-ending.

(c) Yamamoto Tsunetomo "Hagakure"

The time is come for a new report of Xterm linting by 🐊v27:

☝️ As usual, any rule can be disabled.

Command used:

putout . --fix

Applied rules:

Current config for 🐊Putout v27:

{
    "rules": {
        "apply-is-array": ["on", {
            "inline": true
        }],
        "apply-comparison-order": "off",
        "new/remove-useless": "off",
        "remove-useless-return": "off",
        "typescript/remove-useless-parens": "off",
        "typescript/remove-useless-types-from-constants": "off",
        "remove-useless-type-conversion/with-double-negations": "off",
        "convert-typeof-to-is-type": "off",
        "apply-shorthand-properties": "off",
        "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",
        "promises/remove-useless-async": "off",
        "simplify-ternary": "off",
        "nodejs/convert-dirname-to-url": "off",
        "nodejs/declare-after-requir": "off",
        "declare-imports-first": "off",
        "apply-array-at": "off",
        "try-catch": "off"
    },
    "match": {
        "*.benchmark.ts": {
            "remove-unused-expressions": "off"
        },
        "SerializeAddon.test.ts": {
            "regexp/optimize": "off"
        },
        "Terminal.ts": {
            "logical-expressions/remove-duplicates": "off"
        }
    },
    "plugins": [
        "apply-shorthand-properties"
    ],
    "ignore": [
        "addons/*/lib/*.js",
        "*.benchmark.js",
        "*.md",
        "*.json",
        "*ignore",
        "*.yml",
        ".npmrc",
        "*.css",
        "out",
        "out-test",
        "demo"
    ]
}

Previous reports:

1
2
3
4
5
6

addons/xterm-addon-search/test/SearchAddon.api.ts

jerch

@coderaiser Went only halfway through and have 2 remarks:

Imho turning left/right side of conditionals is mostly unwanted. I think we want the local changing part on the left side, comparing to a more static right side.
Some conditionals do border testing against a left and a right side. Imho thats easier to grasp, if the the numbers stay in mathematically logical order. (thus left < var < right, which translates to left < v && v < right in JS).

If we can agree on my remarks above, I think the changelist will get much shorter.

jerch · 2022-09-15T10:34:16Z

addons/xterm-addon-canvas/src/GridCache.ts

@@ -4,7 +4,7 @@
 */

 export class GridCache<T> {
-  public cache: (T | undefined)[][];
+  public cache: T[] | undefined[][];


I think this got the dimension wrong on T? Should be T[][] | undefined[][]?

Plus this doesn't feel like an improvement to me

This transformation decrease cognitive load while reading the code. I can turn it of, that's not a problem :). But about benefit, here it is:

let info: (Info | SuperInfo | MagicInfo)[];

vs

let info: Info[] | SuperInfo[] | MagicInfo[];

In first case you need to unwrap types in your brain while reading the code. In second case you just reading the type information without unnecessary unwrapping the code.

I find the former easier to read and less error prone; info is an array of things, where things = Info | SuperInfo | MagicInfo

jerch · 2022-09-15T10:39:48Z

addons/xterm-addon-search/src/SearchAddon.ts

-        result.col + result.term.length >= this._terminal.cols ? result.row + 1 : result.row,
-        result.col + result.term.length >= this._terminal.cols ? 0 : result.col + 1,
+        this._terminal.cols <= result.col + result.term.length ? result.row + 1 : result.row,
+        this._terminal.cols <= result.col + result.term.length ? 0 : result.col + 1,


Over the years I've develop a habit/tendency for conditionals to write the changing entity on the left side comparing to something more statically on the right side, thus I'd prefer the old variant here. In the end its a matter of taste.

Same for several more occasions down below...

Not a fan too, this rule would best be ignored

jerch · 2022-09-15T10:42:38Z

addons/xterm-addon-search/test/SearchAddon.api.ts

@@ -311,7 +311,7 @@ describe('Search Tests', function (): void {
            .replace(/\n/g, '\\n\\r');
        }
        fixture = fixture
-          .replace(/'/g, `\\'`);
+          .replace(/'/g, `'`);


This looks wrong to me compared to the original.

This is a code scanning alert for the PR now, it does nothing in its current state

jerch · 2022-09-15T10:47:02Z

addons/xterm-addon-webgl/src/atlas/WebglCharAtlas.ts

-          Math.abs(imageData.data[offset + 1] - g) +
-          Math.abs(imageData.data[offset + 2] - b)) < threshold) {
+          threshold > (Math.abs(imageData.data[offset] - r) +
+          Math.abs(imageData.data[offset + 1] - g) + Math.abs(imageData.data[offset + 2] - b))) {


Basically same as above. Additionally if readability is an issue here, maybe introduce an interim variable? (No issue for me though...)

jerch · 2022-09-15T10:51:20Z

bin/test_mousemodes.js

@@ -96,7 +96,7 @@ function evalButtonCode(code) {
  }
  if (move) {
    action = 'move';
-  } else if (4 <= button && button <= 7) {
+  } else if (button >= 4 && button <= 7) {


Ok, here comes the exclusion from my habit above - if things test left/right borders, I prefer to write it with numbers to left and right side and variable in the middle. Always found that easier to grasp, visually resembling the "in the middle" aspect. Thus I'd prefer the original variant here.

I do this too as it's similar to 4 <= button <= 7 in maths

jerch · 2022-09-15T10:52:12Z

src/browser/renderer/RendererUtils.ts

 }

 function isBoxOrBlockGlyph(codepoint: number): boolean {
-  return 0x2500 <= codepoint && codepoint <= 0x259F;
+  return codepoint >= 0x2500 && codepoint <= 0x259F;


Again left/right border testing...

jerch · 2022-09-15T10:55:28Z

src/browser/renderer/dom/DomRenderer.ts

@@ -231,11 +231,11 @@ export class DomRenderer extends Disposable implements IRenderer {
      ` background-color: ${this._colors.selectionInactiveBackgroundOpaque.css};` +
      `}`;
    // Colors
-    this._colors.ansi.forEach((c, i) => {
+    for (const [i, c] of this._colors.ansi.entries()) {


Yes much better, and since we are now on ES2015, should be officially supported. Also the for statement is way faster in general than the lisp style function mappers.

Another benefit is this works properly when debugging, instead of needing a breakpoint in the loop block

src/common/Color.ts

jerch · 2022-09-15T11:05:59Z

src/common/InputHandler.ts

@@ -2286,7 +2286,7 @@ export class InputHandler extends Disposable implements IInputHandler {
            cSpace = 1;
          }
          accu[advance + i + 1 + cSpace] = subparams[i];
-        } while (++i < subparams.length && i + advance + 1 + cSpace < accu.length);
+        } while (subparams.length > ++i && accu.length > i + advance + 1 + cSpace);


I would not touch this and the following similar cases, as the execution order is very important. (The inline ++i in a conditional would even generate a warning in C/C++ for that reason).

coderaiser · 2022-09-15T14:26:40Z

Just disabled unneeded rules :).

src/common/services/InstantiationService.ts

github-advanced-security bot found potential problems Sep 14, 2022

View reviewed changes

addons/xterm-addon-search/test/SearchAddon.api.ts Fixed Show fixed Hide fixed

jerch reviewed Sep 15, 2022

View reviewed changes

chore: lint using 🐊Putout

25eb7d6

coderaiser force-pushed the feature/lint-by-putout branch from cfc1ed2 to 25eb7d6 Compare September 15, 2022 14:19

Tyriar requested changes Sep 15, 2022

View reviewed changes

src/common/services/InstantiationService.ts Outdated Show resolved Hide resolved

coderaiser commented Sep 15, 2022

View reviewed changes

src/common/services/InstantiationService.ts Outdated Show resolved Hide resolved

coderaiser added 2 commits September 15, 2022 22:52

Update src/common/services/InstantiationService.ts

bbff3e8

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

497cfcb

coderaiser requested a review from Tyriar September 19, 2022 09:00

Tyriar self-assigned this Oct 4, 2022

Tyriar approved these changes Oct 4, 2022

View reviewed changes

Tyriar added this to the 5.1.0 milestone Oct 4, 2022

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

f6a61ac

Tyriar enabled auto-merge October 4, 2022 16:52

Tyriar merged commit e1e8f28 into xtermjs:master Oct 4, 2022

coderaiser deleted the feature/lint-by-putout branch July 14, 2023 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint using 🐊Putout: part 7 #4118

Lint using 🐊Putout: part 7 #4118

coderaiser commented Sep 14, 2022 •

edited

jerch left a comment •

edited

jerch Sep 15, 2022

Tyriar Sep 15, 2022

coderaiser Sep 15, 2022

Tyriar Sep 15, 2022

jerch Sep 15, 2022

Tyriar Sep 15, 2022

jerch Sep 15, 2022

Tyriar Sep 15, 2022

jerch Sep 15, 2022

jerch Sep 15, 2022

Tyriar Sep 15, 2022

jerch Sep 15, 2022

jerch Sep 15, 2022

Tyriar Sep 15, 2022

jerch Sep 15, 2022

coderaiser commented Sep 15, 2022

Lint using 🐊Putout: part 7 #4118

Lint using 🐊Putout: part 7 #4118

Conversation

coderaiser commented Sep 14, 2022 • edited

jerch left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderaiser commented Sep 15, 2022

coderaiser commented Sep 14, 2022 •

edited

jerch left a comment •

edited