Update 'convert to raw string' to help cleanup strings with leading whitespace #60936
Conversation
|
Will this be able to be applied solution-wide? I remember some fixes don't automatically get this ability. |
We should be able to after we merge #60906 and enable FixAll support for this specific refactoring. This is the exact thing that @CyrusNajmabadi suggested to me yesterday while reviewing that PR. |
...edUtilitiesAndExtensions/Compiler/Core/EmbeddedLanguages/VirtualChars/VirtualCharSequence.cs
Outdated
Show resolved
Hide resolved
| <comment>This clause is a follow up to the "Convert to raw string" loc string. | ||
| The intent is that the user sees "Convert to raw string" as an option to select, | ||
| and that is then followed with this clause. This is so we don't have a huge string | ||
| saying "Convert to raw string without leading whitespace (may change semantics)"</comment> |
There was a problem hiding this comment.
do we need "(may change semantics)"? I would think that a) removing leading whitespace would imply that and b) its a refactoring so could change semantics
There was a problem hiding this comment.
i do think it's important (personally). Otherwise, i worry that someone may be confused about waht "indentation" means. e.g. if it's the content indentation that is adjusted, or the syntax indentation is adjusted.
| var length = Math.Min(leadingWhitespace1.Length, leadingWhitespace2.Length); | ||
|
|
||
| var current = 0; | ||
| while (current < length && IsCSharpWhitespace(leadingWhitespace1[current]) && leadingWhitespace1[current].Rune == leadingWhitespace2[current].Rune) |
There was a problem hiding this comment.
do we care about cases of mixed tabs/spaces?
There was a problem hiding this comment.
I'm ok with thsi only trimming off the part it is certain is shared across the files. If you mix tabs/spaces... well... that's either rare or bizarre, and i'm ok ignoring :)
| return commonLeadingWhitespace.Length; | ||
| } | ||
|
|
||
| private static VirtualCharSequence ComputeCommonWhitespacePrefix( |
There was a problem hiding this comment.
could this just be an int?
Where you consume the current line text up until either the current 'common' index or a non-whitespace char?
Or maybe just a simple extension to virtual char that is GetFirstNonWhitespaceIndex and then compare the indices? Think that could replace some other helpers (AllWhitespace) as well
There was a problem hiding this comment.
i'll see if i can do that! :)
There was a problem hiding this comment.
ah, this can't be an int. if it's an int, we can't distinguish spaces/tabs.
| } | ||
|
|
||
| // Remove all trailing whitespace and newlines from the final string. | ||
| while (result.Count > 0 && (IsCSharpNewLine(result[^1]) || IsCSharpWhitespace(result[^1]))) |
There was a problem hiding this comment.
wasn't this done here?
https://github.com/dotnet/roslyn/pull/60936/files#diff-0428e947f9429e42713669adec6cd527db6d211b4197c9fdf2b6af0cf55cd77aR190
We can't add more trailing whitespace as part of skipping the common ones can we?
There was a problem hiding this comment.
it's slightly different. the first loop is removing entirely blank lines. The last loop is removing any trailing newline from the last line (which is not an allwhitespace line (since htat would have already been removed)).
| return line.GetSubSequence(TextSpan.FromBounds(0, current)); | ||
| } | ||
|
|
||
| private static void BreakIntoLines(VirtualCharSequence characters, ArrayBuilder<VirtualCharSequence> lines) |
There was a problem hiding this comment.
should virtualcharsequence have a split extension that takes in the characters to split on?
There was a problem hiding this comment.
possibly. though \r\n would be a PITA to have to deal with :(
| lines.RemoveAt(lines.Count - 1); | ||
|
|
||
| if (lines.Count == 0) | ||
| return VirtualCharSequence.Empty; |
There was a problem hiding this comment.
Should this check be moved up and prevent the fix from being offered? I can't think of why someone would have a bunch of whitespace is a verbatim string, but offering to convert it to an empty string is probably not what they want to happen.
There was a problem hiding this comment.
fair point. Updated to not offer in that case.
...AndExtensions/Compiler/Core/EmbeddedLanguages/VirtualChars/VirtualCharSequence.Enumerator.cs
Show resolved
Hide resolved
ghost
added this to the 
Fixes: #59591
This is for the case where someone writes code like so:
With teh existing feature you would get:
Which is not ideal. Now, we offer two options here. The normal semantic-preserving change, but also a new semantic-changing option:
Choosing this will now produce:
Which now matches the user's intent here for their particular domain.