-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(typescript-estree): options range loc being always true #704
Conversation
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.
I don't think there's a problem with the idea behind your logic, but I think that it'll be a bit hard on the maintenance side.
We will have to ensure that we remember to catch every single place that creates a child node outside of the normal converter
flow, and call applyExtraOptions
to it.
This is very manual and will be error prone. For example in this diff, it looks like you missed the usages in convertJSXTagName
.
I think a better option would be to do this after the AST has been created in its entirety.
Within the parser
package, we have a list of visitor keys. Additionally, we have a utility class, SimpleTraverser
, which you can use to traverse the AST.
These two could be moved into typescript-estree
, and used to traverse the tree after the fact, and scrub the range/loc if requested:
function parse(file, options) {
const ast = convert();
if (options.range === false || options.loc === false) {
const traverser = new SimpleTraverser({
enter() => // scrub range and/or loc
})
traverser.traverse(ast);
}
}
WDYT?
Sure I think it's better to do this after the AST has been created. I'll make the changes |
Codecov Report
@@ Coverage Diff @@
## master #704 +/- ##
==========================================
+ Coverage 93.98% 93.98% +<.01%
==========================================
Files 121 121
Lines 5217 5224 +7
Branches 1444 1447 +3
==========================================
+ Hits 4903 4910 +7
Misses 179 179
Partials 135 135
|
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.
LGTM! Thanks for doing this
Fix #694
@bradzacher
Originally planned on removing
range
andloc
inside heretypescript-eslint/packages/typescript-estree/src/convert.ts
Lines 207 to 210 in c2ad091
but saw that some Nodes(
StringLiteral
,BigIntLiteral
) depended onrange
aftercreateNode
.Left the responsibility of removing it to
typescript-eslint/packages/typescript-estree/src/convert.ts
Lines 83 to 88 in c2ad091
which covered all Nodes except
ClassDeclaration
,InterfaceDeclaration
,ShorthandPropertyAssignment
on which I had to remove the field if necessaryLet me know if I need to change anything