Ensure source paths are comparable with editorconfig directory paths #73100
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| normalizedDirectory = PathUtilities.NormalizeWithForwardSlash(normalizedDirectory); | ||
| normalizedDirectory = PathUtilities.ExpandAbsolutePathWithRelativeParts(normalizedDirectory); | ||
|
|
||
| var normalizedPath = PathUtilities.EnsureTrailingSeparator(normalizedDirectory) + Path.GetFileName(sourcePath); |
There was a problem hiding this comment.
The EnsureTrailingSeparator call is going to add an OS specific separator while I believe this code path expects to have forward slashes.
There was a problem hiding this comment.
EnsureTrailingSeparator looks for existing slashes in the path and adds a consistent one, so that shouldn't be a problem.
jjonescz
force-pushed
the
72657-DoubleSlash
branch
from
087d429
to
7ba5450
Compare
| /// <summary> | ||
| /// Replaces all sequences of '\' or '/' with a single '/' but preserves UNC prefix '//'. | ||
| /// </summary> | ||
| public static string CollapseWithForwardSlash(string p) |
There was a problem hiding this comment.
| public static string CollapseWithForwardSlash(string p) | |
| public static string CollapseWithForwardSlash(ReadOnlySpan<char> p) |
Consider using a ReadOnlySpan<char>. Makes it easier for future changes that are span related to call this API.
| { | ||
| sb.Append('/'); | ||
| } | ||
| wasDirectorySeparator = true; |
There was a problem hiding this comment.
Consider an input string of a\\\b. That would result in an output of a/b Is that intended here? If so let's make sure there is a unit test for that case.
There was a problem hiding this comment.
That's intended, will add a test, thanks.
|
@dotnet/roslyn-compiler for reviews, thanks |
|
@RikkiGibson, @chsienki PTAL |
| @@ -210,6 +210,63 @@ class C | |||
| Assert.Null(cmd.AnalyzerOptions); | |||
| } | |||
|
|
|||
| [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/72657")] | |||
| public void AnalyzerConfig_DoubleSlash(bool doubleSlash1, bool doubleSlash2) | |||
There was a problem hiding this comment.
nit: consider naming these parameters based on which element we are introducing double slashes for, e.g. bool doubleSlashAnalyzerConfig, bool doubleSlashSource
| [InlineData("/a/b/c/", "/a/b/c//")] | ||
| [InlineData("/a/b/c//", "/a/b/c//")] | ||
| [InlineData("/a/b//c/", "/a/b///c/")] | ||
| public void EditorConfigToDiagnostics_DoubleSlash(string prefix1, string prefix2) |
There was a problem hiding this comment.
Consider naming these parameters based on how each prefix is being used.
* upstream/main: (1047 commits) Ensure source paths are comparable with editorconfig directory paths (dotnet#73100) Lower drop retention from 10 years to 3 months (dotnet#73190) Move fix restore code remove simplify simplify Move off of map Simplify tests Simplify tests Simplify tests Simplify tests Simplify tests simplify finish Simplify Fix Finish Remove dispose support ...
Fixes #72657 as suggested in comment #72657 (comment).