Improve 'convert regular to raw-string' for common cases (like test code). #67060
Conversation
| } | ||
|
|
||
| [Fact] | ||
| public async Task FixAllCommonRoslynTestPattern1() |
There was a problem hiding this comment.
these are the new behaviors.
| class Y | ||
| { | ||
| } | ||
| """); |
There was a problem hiding this comment.
this output is much better than before. both arguments are now properly indented, while the contents of that argument stay appropriately non-indented.
| var indentationOptions = new IndentationOptions(formattingOptions); | ||
|
|
||
| var tokenLine = parsedDocument.Text.Lines.GetLineFromPosition(token.SpanStart); | ||
| if (token.SpanStart == tokenLine.Start) |
There was a problem hiding this comment.
this is the new logic.
| var parsedDocument = await ParsedDocument.CreateAsync(document, cancellationToken).ConfigureAwait(false); | ||
| var indentation = token.GetPreferredIndentation(parsedDocument, indentationOptions, cancellationToken); | ||
| return ConvertToMultiLineRawIndentedString(indentation, token, formattingOptions, characters); | ||
| SyntaxToken ConvertToSingleLineRawString() |
There was a problem hiding this comment.
this helper was inlined.
|
|
||
| SyntaxToken ConvertToMultiLineRawIndentedString( | ||
| string indentation, | ||
| bool addIndentationToStart) |
There was a problem hiding this comment.
this helper was inlined. though the addIndentationToStart part is new. it represents the new behavior we want.
| @@ -162,21 +163,30 @@ private static bool CanConvertStringLiteral(SyntaxToken token, out CanConvertPar | |||
| // This changes the contents of the literal, but that can be fine for the domain the user is working in. | |||
| // Offer this, but let the user know that this will change runtime semantics. | |||
| canBeMultiLineWithoutLeadingWhiteSpaces = token.IsVerbatimStringLiteral() && | |||
| HasLeadingWhitespace(characters) && | |||
| (HasLeadingWhitespace(characters) || HasTrailingWhitespace(characters)) && | |||
There was a problem hiding this comment.
discovered as we have a lots of tests that use:
@"class C
{
}
"This allows the user to say they want to convert, while removing that trailing newline. So you get:
"""
class C
{
}
"""not
"""
class C
{
}
"""
There was a problem hiding this comment.
I agree that we want to offer this fix, but I find it exceedingly unfortunate that it was applied in bulk #67059. I would have expected that bulk refactoring to avoid any behavior changes.
There was a problem hiding this comment.
I disagree. Applying in bulk without the behavior change just leads to horrendous outcomes with tests full of unnecessary whitespace that was not part of the original test's motivations.
That whitespace was there because of how c# literals work, not because the test author wanted them.
The conversion (which you have to opt into) does change the strict c# meaning of the strings. But it actually makes the tests reflect what the test author was intending.
|
Does this fix #61267 ? |
|
Why yes it does. how convenient :) |
ghost
added this to the 
Fixed #61267
I was using this feature to update some of our tests to be raw-strings, and found the behavior suboptimal for common patterns we use. Specifically, we often left-align the original string to keep the quotes aligned with the left-aligned content. Keeping things that way when converting to raw strings forces everything to stay left-aligned, evne though it's no longer necessary (as raw strings already handle indentation for you). See tests for examples of the improvements.
You can see the results of this in: #67059