Support autofix in gts files by patricklx · Pull Request #1853 · ember-cli/eslint-plugin-ember

patricklx · 2023-05-08T11:49:51Z

copied some code from https://github.com/typed-ember/glint/blob/main/packages/core/src/language-server/util/position.ts
:) and adapted for better perf

this needs #1852 for tests to pass

hmajoros · 2023-05-09T13:53:15Z

lib/preprocessors/glimmer.js

    );
  }

+  const originalDoc = new DocumentLines(original);


only comment is that we should leave a note to refactor the rest of this file to use the new DocumentLines util!

here its just to transform the offset to original line/column. if the fix range would have used line/columns, no change would be needed.
the line/column modifications later are to move the eslint messages that are inside the template, where line/column actually changed. So I don't think this can be used there

hmajoros · 2023-05-09T13:53:37Z

lib/preprocessors/glimmer.js


 const TRANSFORM_CACHE = new Map();
 const TEXT_CACHE = new Map();
+const RULE_CACHE = new Map();


will this be used in a future PR?

no, it can be removed, I do it once #1852 is merged

NullVoxPopuli

<3

NullVoxPopuli · 2023-05-12T23:33:59Z

lib/utils/document.js

+  //     \u2029              Paragraph separator     <PS>
+  // Only the characters in Table 3 are treated as line terminators. Other new line or line
+  // breaking characters are treated as white space but not as line terminators.
+  return (


huh -- there are more of these than I thought 😅

NullVoxPopuli · 2023-05-12T23:36:25Z

lib/preprocessors/glimmer.js


 const TRANSFORM_CACHE = new Map();
 const TEXT_CACHE = new Map();
+const RULE_CACHE = new Map();


don't forget unused dealio <3

NullVoxPopuli · 2023-05-12T23:37:35Z

lib/utils/document.js

+ * @typedef {{ line: number; character: number }} Position
+ */
+
+class DocumentLines {


This little util class is nifty! nice work!

NullVoxPopuli · 2023-05-12T23:38:18Z

tests/lib/rules-preprocessor/gjs-gts-processor-test.js

      },
    ],
  },
+  {


is there a way to add the fixed output to this test? is that worth it?

You mean the "file content" after auto fix? Not sure if that's worth it. That would just test eslint auto fix capability... Just need to be sure that the fix range is correct.

patricklx · 2023-05-15T11:52:31Z

@bmish this is ready

bmish

Is all of this new logic tested? With jest --coverage, I see untested logic in lib/preprocessors/glimmer.js, but some of that appears to be pre-existing. CC: @hmajoros @NullVoxPopuli

bmish · 2023-05-15T13:58:54Z

lib/preprocessors/glimmer.js

  });

  return filtered.map((message) => {
+    if (message.fix) {


Can you add any high-level comment to provide some context about our support for autofixing and how it works?

hmajoros · 2023-05-15T18:33:38Z

#1853 (review)

I didn't know this was a thing when I developed my stuff 🙈 likely pre-existing. does this not run as part of CI?

patricklx · 2023-05-16T07:48:39Z

Is all of this new logic tested? With jest --coverage, I see untested logic in lib/preprocessors/glimmer.js, but some of that appears to be pre-existing. CC: @hmajoros @NullVoxPopuli

i checked the coverage on v11.5.2 https://github.com/ember-cli/eslint-plugin-ember/blob/v11.5.2/lib/preprocessors/glimmer.js
and on this PR. the code parts not covered remain the same
v11.5.2: 49,120,133,193-196,205
this: 56,128,173,264-267,276

bmish

Thanks!

NullVoxPopuli · 2023-05-16T19:04:22Z

I don't think this fixed the issue 🤔

NullVoxPopuli · 2023-05-16T19:05:03Z

or no! it did! I did something silly.

Issue resolved 🎉

* master: (88 commits) Release 11.7.0 build(deps-dev): Bump @typescript-eslint/parser from 5.59.5 to 5.59.6 (ember-cli#1863) Account for only having template tag in `no-empty-glimmer-component-classes` rule (ember-cli#1866) Support autofix of numerical property access and ternary expressions in `no-get` rule (ember-cli#1865) Release 11.6.0 build(deps-dev): Bump prettier from 2.8.7 to 2.8.8 (ember-cli#1842) build(deps-dev): Bump @typescript-eslint/parser from 5.58.0 to 5.59.5 (ember-cli#1860) build(deps-dev): Bump @babel/eslint-parser from 7.21.3 to 7.21.8 (ember-cli#1856) build(deps-dev): Bump markdownlint-cli from 0.33.0 to 0.34.0 (ember-cli#1848) build(deps-dev): Bump jquery from 3.6.4 to 3.7.0 (ember-cli#1861) build(deps-dev): Bump release-it from 15.10.1 to 15.10.3 (ember-cli#1859) build(deps): Bump vm2 from 3.9.17 to 3.9.18 (ember-cli#1862) Support autofix in gts files (ember-cli#1853) Only show `no-undef` errors for templates in gts files (ember-cli#1852) Release 11.5.2 Fix a bug in autofixer and autofix additional cases with `firstObject and `lastObject` in `no-get` rule (ember-cli#1841) build(deps-dev): Bump eslint from 8.37.0 to 8.38.0 (ember-cli#1830) build(deps-dev): Bump @typescript-eslint/parser from 5.57.0 to 5.58.0 (ember-cli#1837) build(deps-dev): Bump typescript from 5.0.3 to 5.0.4 (ember-cli#1832) build(deps): Bump vm2 from 3.9.11 to 3.9.17 (ember-cli#1839) ...

hmajoros reviewed May 9, 2023

View reviewed changes

patricklx mentioned this pull request May 11, 2023

Only show no-undef errors for templates in gts files #1852

Merged

bmish added the enhancement label May 12, 2023

bmish changed the title ~~support gts autofix~~ Support gts autofix May 12, 2023

bmish changed the title ~~Support gts autofix~~ Support autofix in gts files May 12, 2023

NullVoxPopuli reviewed May 12, 2023

View reviewed changes

patricklx closed this May 13, 2023

patricklx force-pushed the patch-2 branch from 6ff7cf8 to 558dab3 Compare May 13, 2023 05:30

patricklx reopened this May 13, 2023

support gts autofix

0e306d6

patricklx force-pushed the patch-2 branch from ec3c1c0 to 0e306d6 Compare May 13, 2023 07:18

bmish reviewed May 15, 2023

View reviewed changes

add comment about fix range

1a19067

bmish approved these changes May 16, 2023

View reviewed changes

bmish merged commit 393ba67 into ember-cli:master May 16, 2023

patricklx deleted the patch-2 branch May 16, 2023 13:56

ynotdraw mentioned this pull request Jun 13, 2023

#43: Upgrade to latest eslint-plugin-ember, remove eslint-disable, and make --fix work again CrowdStrike/ember-toucan-core#181

Merged

Uh oh!

Conversation

patricklx commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patricklx May 9, 2023 • 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!

NullVoxPopuli left a comment

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!

patricklx May 13, 2023 • 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!

patricklx commented May 15, 2023

Uh oh!

bmish left a comment

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!

hmajoros commented May 15, 2023

Uh oh!

patricklx commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmish left a comment

Choose a reason for hiding this comment

Uh oh!

NullVoxPopuli commented May 16, 2023

Uh oh!

NullVoxPopuli commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patricklx commented May 8, 2023 •

edited

Loading

patricklx May 9, 2023 •

edited

Loading

patricklx May 13, 2023 •

edited

Loading

patricklx commented May 16, 2023 •

edited

Loading

NullVoxPopuli commented May 16, 2023 •

edited

Loading