Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 for generic ASTs #56
Support for generic ASTs #56
Changes from 1 commit
e1a75d6
dd18e9f
ec4e6b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 it worth mentioning the "All Nodes" requirements for
range
andloc
properties and explicitly noting that this RFC would keep those as-is?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.
Sure. Do you have an alternative for keeping those as-is? I just struggled with this, since GraphQL parser returns locations in character offset from the start of the file, vs. ESLint requiring columns and lines. Translating between those was pretty painful, so if there's an alternative you can think of, I would be all for that!:-)
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.
As much as it would open up the ability to use existing third-party parsers, I don't think it's practical to remove this requirement. Rules frequently reference
node.loc
andnode.range
, so there's no way we'll easily get away from having line and column information available somehow. We could allow passinggetLoc(node: ASTNode) => SourceLocation
andgetRange(node: ASTNode) => [number, number]
, but I don't think that's a great idea either. It does mean a parser wouldn't have to do a pre-traversal adding translatedloc
/range
to the AST, but developers would still have to implement that translation algorithm, it just moves to on-demand rather than up-front. My gut says that the overhead of a function call relative to property access would kill performance unless it turns out that we only need location info for a minority of nodes making on-demand less expensive than up-front. About the only thing I like about that idea is that we could do it while maintaining backwards compatibility.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.
Thought: If this is a function, maybe it should have an action name? I'm still partial to
getNodeType()
, but I'm not opposed to something else. Also, I suppose this could be allowed to be a function or a string for simplicity?Not critical, mostly posted here for others' thoughts.
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.
Sure, that sounds like a good enhancement to me, I'll make the change, since most parsers probably wouldn't need support for variable node name. In terms of name, I think
getNodeType
is a bit misleading, for me, for example, it would mean given the node, get me it's type, not the get me the property that identifies all nodes. We could change it to getNodeIdentifier(), but that just feels overly verbose to me.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.
Ah, I see the confusion. To me, we need
getNodeType(node)
to return the node type string (like "Program") rather than just returning the property name. As in the TypeScript example I mentioned, there's just no way to get the node type unless you calculate it, which means we can't just return a property to go look it up.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.
So what you are saying is that
getNodeType
function would take an ASTNode and return back a string representing the type of that node, because TypeScript ASTNodes do not contain identifying property? Am I understanding this correctly? My biggest concern here would be performance implication of this change, since for every single node we wold have to call into the parser. But I guess that could be optimized later. I'm also thinking that if we makegetNodeType
be either a function or a string, the name will be somewhat confusing when it's of type string. But it's a minor concern.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.
Yes, that's correct. If we want to still allow a string to be specified, then we can probably keep the name as
nodeIdentifier
(or maybenodeType
).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.
We could link this to https://github.com/eslint/espree with
[espree](https://github.com/eslint/espree)
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.
Yes, and I think that's actually preferable. It's my impression that we all sort of stumbled onto the realization around the same time that ESLint is closer to supporting non-ESTree ASTs than we might have thought before investigating. If we keep this as experimental, void-your-warranty type stuff, then we get to see where people want to take it rather than building the "perfect" design in an ivory tower before we know what the requirements will be.
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.
Maybe eventually, but I feel safe waiting to see if there's demand for such a thing before implementing it, and I don't see any reason we couldn't do that in a semver-minor change.