Improves inline comments #513
Conversation
|
tomasnyberg
left a comment
tomasnyberg
left a comment
There was a problem hiding this comment.
Tried this out a bit and I generally think it works pretty well, definitely better than the baseline. I think it would be okay to merge, though it does make the layout engine more complex, and I'm slightly concerned about weird edgecases with this type of handling. There's a lot of nitty-gritty logic, which has been troublesome in the past in the formatter.
Some nits for contrived edgecases:
- If there are two inline comments that get put together due to chunking, they do not get spaces between them:
match (n /* comment*/)-/*another comment*/[r]-(m)
return n
// turns into
MATCH (n)- /* comment*//*another comment*/ [r]-(m)
RETURN n
- This PR seems to want spaces around the inline comments always (not exactly sure why?), but it doesn't always happen. I'm a bit skeptical of this line:
if (chunk.type === 'REGULAR' && chunk.noSpace) {
MATCH (n)-[/*comment*/r]-(m)
RETURN n
// turns into
MATCH (n)-[ /*comment*/r]-(m)
RETURN n
| state.formatted += chunk.comment; | ||
| state.column += chunk.comment.length; | ||
| // Always include space after, even if the chunk has noSpace | ||
| if (chunk.type === 'REGULAR' && chunk.noSpace) { |
There was a problem hiding this comment.
not sure how something can be a regular chunk if it is an inline comment? Why is it not a commentchunk?
There was a problem hiding this comment.
No an inline comment is part of the regular chunk. It is only the comment which are completely on their own rows which are comment chunks.
There was a problem hiding this comment.
Now when I look at it, you might be able to do chunk?.noSpace instead, unsure though and not really able to test anymore
| function handleComments(state: State, chunk: Chunk) { | ||
| if (isInlineComment(chunk)) { | ||
| // Inline comment - append directly | ||
| state.formatted += ' '; |
There was a problem hiding this comment.
why are spaces added here if they are also added before and after the inlineComment in formattingHelpers line 170?
There was a problem hiding this comment.
I think you are confusing the debug text with this one. In formattingHelpers the space are added for debug and to the group size, while it is added here to be included in the result.
I think there's no way around this if we want to support a third comment type. Also, subjectively speaking, I think it's only marginally more complex, if at all. Adding about 10 lines to the layout engine is actually quite good for adding support for a third comment type. But you're right to be concerned about adding engine complexity—it's a warranted concern.
Haha, yes you're right. Though I think this is a really contrived example where it's unclear what the best way to handle this actually is, so I'd be happy leaving it as is.
I followed exactly how Prettier does it in this regard and when I experimented with Prettier's spacing, there were some occasions where even they seem to fail to put spaces around comments. There are definitely examples where you could question the result of the inline comments, but in my opinion, optimizing for the last 10% is not really worth here. Especially when it is unclear exactly how it should be done. |
ncordon
force-pushed
the
improves-inline-comments
branch
from
2362ab5 to
6b293a5
Compare
3 participants
Description
This PR fixes the tech debt on inline comments from PR #460. The tech debt is that an inline comment (
/* Comment*/)should not be treated as regular comments (// Comment).How it works
In comparison to a regular comment which should be appended before a new line, an inline comment can be inserted basically anywhere. It can not split a chunk however, meaning an inline comment will be placed first after a chunk. The inline comment also counts towards the group size, I took this from Prettier which seems to be doing similar thing.
Also similar to Prettier an inline comment should always have a space before and after. This makes calculating group size a bit more complicated.
Tests