Only show `no-undef` errors for templates in gts files by patricklx · Pull Request #1852 · ember-cli/eslint-plugin-ember

patricklx · 2023-05-04T13:03:08Z

this filters out any linting message that is within the template, except for the no-undef rule.
there are also errors like Expected blank line between class members, which wrap the whole template block, which will be included

NullVoxPopuli · 2023-05-06T12:33:22Z

Can tests be added? Thanks!

Also, is no-undef really the only lint we care about? 😅

patricklx · 2023-05-06T12:40:56Z

Yes, according to this comment:

eslint-plugin-ember/lib/preprocessors/glimmer.js

Line 88 in 208e6b6

// value and allow-list the rules that check for undefined identifiers

NullVoxPopuli · 2023-05-06T14:30:18Z

hmm I wrote that. 🙃

Sounds good then -- ping when you push up some tests or if you have any questions -- will be good to get this merged!

patricklx · 2023-05-08T07:57:48Z

@NullVoxPopuli @bmish its now ready, the tests would now fail without the change in glimmer.js

NullVoxPopuli · 2023-05-08T11:10:28Z

Nice, lgtm then. @bmish , can you approve the workflow run?

bmish · 2023-05-08T14:21:00Z

lib/preprocessors/glimmer.js

-  return flattened.map((message) => {
+  const filtered = flattened.filter(
+    (it) =>
+      it.ruleId === 'no-undef' ||


Can you comment to explain why no-undef is the only rule included here?

i think @NullVoxPopuli should answer :)

Is the logic here "ember-template-lint should give us errors from inside a template context"?

are we certain there will not be other eslint rules written in the future that may require us to update this logic?

I think it's the only rule that makes sense on a transformed code. The only thing i can think of is when template-lint would become a eslint plugin

@hmajoros exactly -- the only thing template-lint can't know about is what is in scope -- so the template-lint equiv of no-undef is disabled for gjs/gts

Waiting for a comment to be added about this.

There is a comment here already:

eslint-plugin-ember/lib/preprocessors/glimmer.js

Line 88 in 208e6b6

// value and allow-list the rules that check for undefined identifiers

Should i move it down to this code?

I think two improvements would help:

Update the comment to explain why we need to "allow-list the rules that check for undefined identifiers".

Move the rule name into an array with a descriptive name like:

const RULES_THAT_FOO = ['no-undef'];

This name is self-documenting and an array would be reusable and easily expandable. Something to this effect.

bmish · 2023-05-08T14:22:09Z

lib/preprocessors/glimmer.js


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


Can you add some high-level comment(s) explaining what's going on and why?

maybe the naming is not ideal. how about TEMPLATE_RANGE_CACHE. it saves the template range so we can know which eslint messages are inside the template ranges in the postprocessing step

i moved the whole thing to postprocessing, as the cache is not really needed

patricklx · 2023-05-10T15:06:34Z

@bmish i pushed a fix, tests are passing.
Edit: ah... Lint is failing

lib/preprocessors/glimmer.js

patricklx · 2023-05-11T08:09:50Z

mmm, i will rework this to use the DocumentLines utils from my other PR, which makes the check more easier.
just noticed the check is not completely correct, would be easier to test with offsets.

bmish · 2023-05-11T14:46:33Z

lib/preprocessors/glimmer.js

+// eslint rules that check for undefined identifiers
+// this are the only rules we want to check within a template


I still would like to see an explanation for why this is the only rule we care about.

there is here:

eslint-plugin-ember/lib/preprocessors/glimmer.js

Line 84 in 208e6b6

// we could accidentally have this line formatted with

.
. Should i add an explanation here too? Like
"Because otherwise eslint would show errors unrelated to the original code and apply autofixes like spacing, comna etc into the template"

Any where we could clarify comments is appreciated. I will leave it to your and @NullVoxPopuli's judgment.

things look good from over here

patricklx · 2023-05-11T20:47:42Z

@bmish fixed, tests pass, full coverage

bmish · 2023-05-11T20:50:25Z

lib/utils/document.js

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


Can you add a high-level comment to this class/file explaining what it's for and where this comes from? Does it need tests? Need to make sure the "what" and "why" is clear for all this new code for someone unfamiliar with the feature.

It comes from here: https://github.com/typed-ember/glint/blob/main/packages/core/src/language-server/util/position.ts

And will also be used for
#1853

bmish · 2023-05-11T20:51:47Z

lib/preprocessors/glimmer.js

+// this are the only rules we want to check within a template
+// Because otherwise eslint would show errors unrelated to the original code and
+// apply auto fixes like spacing, comma etc. into the original template
+const RULES_THAT_CHECK_FOR_UNDEFINED_IDENTIFIERS = new Set(['no-undef']);


I know I suggested RULES_THAT_CHECK_FOR_UNDEFINED_IDENTIFIERS but then I realized it's a bad name since it could only ever apply to this one rule. Can we use a better name for this array that explains what kind of rules would qualify or what the array is used for?

patricklx · 2023-05-12T11:37:00Z

Coverage was not enough. I think because of the document utility class.
Can we disable the coverage for the document utility? It has a function which i do not use now. But i use it in the other pr.
I can try to enable it there again

bmish

Thanks, merging to unblock #1853.

* 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) ...

patricklx force-pushed the patch-1 branch 3 times, most recently from c889e14 to 32b1910 Compare May 4, 2023 13:53

patricklx force-pushed the patch-1 branch from bbc40f7 to b606882 Compare May 8, 2023 07:55

patricklx mentioned this pull request May 8, 2023

Support autofix in gts files #1853

Merged

bmish reviewed May 8, 2023

View reviewed changes

patricklx force-pushed the patch-1 branch 2 times, most recently from 7939052 to 909d1bc Compare May 10, 2023 07:26

bmish reviewed May 10, 2023

View reviewed changes

lib/preprocessors/glimmer.js Show resolved Hide resolved

patricklx force-pushed the patch-1 branch from c89a181 to 239a80e Compare May 10, 2023 15:21

bmish added the bug label May 10, 2023

bmish reviewed May 11, 2023

View reviewed changes

only show no-undef errors for templates in gts files

eee5cc2

patricklx force-pushed the patch-1 branch from 7ccad7c to eee5cc2 Compare May 11, 2023 20:47

bmish reviewed May 11, 2023

View reviewed changes

patricklx added 2 commits May 11, 2023 23:05

rename

04811ab

add documentation

9ce0303

bmish changed the title ~~only show no-undef errors for templates in gts files~~ Only show no-undef errors for templates in gts files May 11, 2023

ignore coverage for document utility

0466f1a

bmish approved these changes May 12, 2023

View reviewed changes

bmish merged commit 558dab3 into ember-cli:master May 12, 2023

patricklx deleted the patch-1 branch May 12, 2023 17:18

		// eslint rules that check for undefined identifiers
		// this are the only rules we want to check within a template

Uh oh!

Conversation

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

Uh oh!

Uh oh!

NullVoxPopuli commented May 6, 2023

Uh oh!

patricklx commented May 6, 2023

Uh oh!

NullVoxPopuli commented May 6, 2023

Uh oh!

patricklx commented May 8, 2023

Uh oh!

NullVoxPopuli commented May 8, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmish May 10, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

patricklx commented May 11, 2023

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 commented May 11, 2023

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 commented May 12, 2023

Uh oh!

bmish left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patricklx commented May 4, 2023 •

edited

Loading

bmish May 10, 2023 •

edited

Loading

patricklx commented May 10, 2023 •

edited

Loading