-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix eol handling on the last token in a file when formatting code actions #79602
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 eol handling on the last token in a file when formatting code actions #79602
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.
c
/// or follows, then we expand to consume at least the full span of the <paramref name="firstToken"/> and | ||
/// <paramref name="lastToken"/> so that we at least will try to format any trivia on them. | ||
/// </summary> | ||
internal static TextSpan GetSpanIncludingPreviousAndNextTokens(SyntaxToken firstToken, SyntaxToken lastToken) |
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.
the thing we're fixing is that we injected a final }
into the file with an elastic-cr-lf (this is good).
However, the formatting engine wasn't ever fixing up that elastic-cr-lf because the code below effectively said "only format up to the exact end of the }
, no further". This was happening because we normally try to find the 'next' token after the last touched token and format through that. But in this case, there is no next token (it's the last token in the file). So the fix is in that case to consider up through the full end of the token (which includes the trailing trivia) so we will update that properly.
@@ -2125,7 +2125,6 @@ public override void AbstractMethod() | |||
throw new System.NotImplementedException(); | |||
} | |||
} | |||
|
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 this because we are handling InsertFinalNewLine
correctly now?
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.
@@ -271,7 +269,7 @@ void Method() | |||
} | |||
} | |||
} | |||
</Document> | |||
</Document> |
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.
What caused this indentation change?
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.
The doc is getting formatted, and tha tcleans up the errant final whitespace we weren't touching before.
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.
Fixes #79584