Improve handling of dangling comments in ForStatement#10059
Improve handling of dangling comments in ForStatement#10059thorn0 wants to merge 23 commits intoprettier:mainfrom
Conversation
4e76287 to
51774a0
Compare
|
Seems a good solution. |
cde2eb0 to
c34a492
Compare
src/language-js/comments.js
Outdated
| } | ||
|
|
||
| function createCommentAnchorPseudoNode(ref, marker, loc) { | ||
| return { type: "Prettier:CommentAnchor", ref, marker, range: [loc, loc] }; |
There was a problem hiding this comment.
Can we use object for this marker? I remeber you already use if for something in class, it should be more clear to understand what it is.
There was a problem hiding this comment.
It's a string label on a dangling comment needed to differentiate it from other dangling comments and to associate it with a specific place inside the enclosing node. Are you proposing not to use strings for this? I think strings work pretty well here: e.g. if the init part of a for statement is omitted, we might have dangling comments with the init marker there.
I remeber you already use if for something in class
For the same thing: to differentiate comments related to extends from comments related to implements, etc.
There was a problem hiding this comment.
I'm thinking about renaming marker to anchor as it seems to convey the purpose better ("something comments are attached to"), but let's do that in another PR. Another idea was attachmentPoint. I chose anchor as a shorter synonym.
There was a problem hiding this comment.
I did some renames, see the last commit.
0a6d307 to
05d2d4b
Compare
c29d1f7 to
2589c65
Compare
fisker
left a comment
There was a problem hiding this comment.
Looks good, but I would prefer to use tokens.
|
@fisker I like tokens too as they're more structured then the text-scanning hacks, but on the other hand depending only on 2 inputs (AST and original text) is better than on 3 (AST, original text, tokens). The resulting code is a bit more universal. |
src/language-js/comments.js
Outdated
|
|
||
| function createCommentAnchorPseudoNode(enclosingNode, name, loc) { | ||
| return { | ||
| type: "Prettier:CommentAnchor", |
There was a problem hiding this comment.
What do you think ::CommentAnchor, like a css pseudo element. 😄
b32ffe3 to
4f09191
Compare
4f09191 to
be00809
Compare
be00809 to
c964222
Compare
Description
Fixes #10046
A better handling for comments like this:
for (/*a*/; /*b*/; /*c*/) ...A proof of concept of an approach that doesn't modify the AST unlike an alternative approach to a similar problem in #9857.
Checklist
docs/directory).changelog_unreleased/*/XXXX.mdfile followingchangelog_unreleased/TEMPLATE.md.✨Try the playground for this PR✨