Do not normalize leading whitespace in code comments#57414
Conversation
src/Workspaces/Core/Portable/Shared/Utilities/DocumentationComment.cs
Outdated
Show resolved
Hide resolved
|
|
||
| private static string TrimEachLine(string text) | ||
| => string.Join(Environment.NewLine, text.Split(s_NewLineAsStringArray, StringSplitOptions.RemoveEmptyEntries).Select(i => i.Trim())); | ||
| { |
There was a problem hiding this comment.
this doesn't seem to be about 'code' blocks at all. It seems to relate to all lines of code (unless i'm missing something). So wouldn't this affect our normalization of code outside of codeblocks? if that's not the case can you explain why (and also doc the code accordingly)?
There was a problem hiding this comment.
so if we had:
<code>
int x;
int y;
</code>
this would trim? But if we had:
BlahBlah
<code>
int x;
int y;
</code>
We wouldn't? that seems odd to me.
There was a problem hiding this comment.
No, in both cases you showed it won't trim.
It will trim:
BlahBlah
<code>
int x;
int y;
</code>
and
<code>
int x;
int y;
</code>
There was a problem hiding this comment.
i'm confused. those are my cases. :) you said it won't trim... but htat it will trim.
There was a problem hiding this comment.
Yours don't have leading whitespace on all lines:
<code>
int x;
int y;
</code>
vs
<code>
int x;
int y;
</code>
| i++; | ||
| } | ||
|
|
||
| maxPrefix = Math.Min(maxPrefix, i); |
There was a problem hiding this comment.
tests that exercising the codepaths with different lengths of maxprefix on different lines (including lines whose prefixes get shorter (or longer) than prior lines).
There was a problem hiding this comment.
The test I've added does have different prefixes on different lines. Happy to add more though.
| while (i < line.Length && char.IsWhiteSpace(line[i])) | ||
| { | ||
| i++; | ||
| } |
There was a problem hiding this comment.
this isn't accurate with tabs vs spaces. i think we can live with that.
| var i = 0; | ||
| while (i < line.Length && char.IsWhiteSpace(line[i])) | ||
| { | ||
| i++; | ||
| } | ||
|
|
||
| // Don't include all-whitespace lines in the calculation | ||
| // They'll be completely trimmed | ||
| if (i == line.Length) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
| var i = 0; | |
| while (i < line.Length && char.IsWhiteSpace(line[i])) | |
| { | |
| i++; | |
| } | |
| // Don't include all-whitespace lines in the calculation | |
| // They'll be completely trimmed | |
| if (i == line.Length) | |
| { | |
| continue; | |
| } | |
| var firstNonWhitespaceOffset = line.GetFirstNonWhitespaceOffset(); | |
| // Don't include all-whitespace lines in the calculation | |
| // They'll be completely trimmed | |
| if (firstNonWhitespaceOffset == null) | |
| continue; |
…lize-leading-whitespace-in-code-comments
|
@CyrusNajmabadi failures look like build flakiness. Is there anything I can do to move this PR forward? |
|
@pakrym I'm on it :-) |
Fixes: #57262