-
-
Notifications
You must be signed in to change notification settings - Fork 5.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
Refactor trailing comment adjustment #10380
Conversation
Following up from babel#10369 - Unify the logic for adjusting trailing comments into a separate function - Use it for all three cases, which fixes a bug for ObjectExpressions and CallExpressions - Update tests to check for the fixed bug
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11437/ |
@@ -38,6 +38,48 @@ export default class CommentsParser extends BaseParser { | |||
this.state.leadingComments.push(comment); | |||
} | |||
|
|||
adjustCommentsAfterTrailingComma_(node: Node, elements: Node[]) { |
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.
Why the trailing underscore?
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.
Oh I wanted to mark it as a private function and out of habit followed the convention from another repo. I don't see a convention in babel so I'll just drop the underscore.
// comma separated list of nodes to be the trailing comments on the last | ||
// element | ||
if (firstChild) { | ||
switch (node.type) { |
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 also SequenceExpression
? (I didn't check)
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 SequenceExpression
will allow trailing comma.
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 skimmed through https://github.com/babel/babylon/blob/master/ast/spec.md and looks like ImportDeclaration
and ArrayPattern
also allow trailing commas. Should I add those?
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.
Yeas please! And also ExportNamedDeclaration
and ObjectPattern
These have to be handled a bit differently, because the node is visited after the and before the declaration. So we intercept when we are going from the last specifier to the source node.
/* One */ Foo | ||
/* Two */, | ||
/* Three */ | ||
} /* Four */ from "foo"; |
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.
Shouldn't "Four" be an innerComment
of ExportNamedDeclaration
?
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.
That would be better, but from what I can tell, there's no way to figure out which comments are before the closing bracket and which ones are after. Before the PR, the last three comments get attached as leadingComments
of foo
, so we were at least not dropping them. So I could remove handling of ImportDeclaration
and ExportDeclaration
from this PR if you prefer the prior logic.
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 tried to call adjustCommentsAfterTrailingComma
right before eating the }
in parseNamedImportSpecifiers
, instead of doing so in processComment
. By doing so, Four
gets attached as a leadingComments
of "foo"
.
I'm ok with either behaviors (leadingComments
of "foo"
or trailingComments
of Foo
); attaching it as an innerComment
is too hard with the current comments attachment logic.
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.
Agree. Comment attachment currently could only be invoked at finishNode
, and it will definitely misclassify some inner comments as long as there are multiple whitespaces between the end
location of any two nodes.
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.
Yeah, looks like something like #9084 is needed to overhaul comment parsing properly, but there might be challenges around backwards compatibility.
I'm slightly more inclined to keep them as trailingComments
on Foo
, because I expect there to be more examples of trailing comments before the }
than after, so I'll leave this in.
if (this.state.leadingComments.length === 0) { | ||
return; | ||
} | ||
if (!this.state.commentPreviousNode) { |
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 think this.state.commentPreviousNode
will always be non-empty as long as elements
is not empty.
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.
Oh yeah good point.
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
Following up from #10369