-
-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support autofix in gts files #1853
Conversation
lib/preprocessors/glimmer.js
Outdated
@@ -135,7 +137,18 @@ function mapRange(messages, filename) { | |||
); | |||
} | |||
|
|||
const originalDoc = new DocumentLines(original); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only comment is that we should leave a note to refactor the rest of this file to use the new DocumentLines util!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
lib/preprocessors/glimmer.js
Outdated
|
||
const TRANSFORM_CACHE = new Map(); | ||
const TEXT_CACHE = new Map(); | ||
const RULE_CACHE = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this be used in a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it can be removed, I do it once #1852 is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
lib/utils/document.js
Outdated
// \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 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh -- there are more of these than I thought 😅
lib/preprocessors/glimmer.js
Outdated
|
||
const TRANSFORM_CACHE = new Map(); | ||
const TEXT_CACHE = new Map(); | ||
const RULE_CACHE = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget unused dealio <3
lib/utils/document.js
Outdated
* @typedef {{ line: number; character: number }} Position | ||
*/ | ||
|
||
class DocumentLines { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This little util class is nifty! nice work!
@@ -206,6 +207,37 @@ const invalid = [ | |||
}, | |||
], | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to add the fixed output to this test? is that worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@bmish this is ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -193,6 +194,14 @@ function mapRange(messages, filename) { | |||
}); | |||
|
|||
return filtered.map((message) => { | |||
if (message.fix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add any high-level comment to provide some context about our support for autofixing and how it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I didn't know this was a thing when I developed my stuff 🙈 likely pre-existing. does this not run as part of CI? |
i checked the coverage on v11.5.2 https://github.com/ember-cli/eslint-plugin-ember/blob/v11.5.2/lib/preprocessors/glimmer.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I don't think this fixed the issue 🤔 |
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) ...
fixes #1750
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