From 4d47cfa1a6046f3a6d487f5f9a2e8afa04534de3 Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Mon, 12 May 2025 13:04:40 +1000 Subject: [PATCH] Minor respath improvements (#5876) * Minor respath improvements * Add helpers * tweak helper * Throw on more than 1 char * comments * No emoji separators --- Robust.Shared/Utility/ResPath.cs | 45 +++++++++++++------ .../Shared/Utility/ResPathTest.cs | 14 ++++-- 2 files changed, 43 insertions(+), 16 deletions(-) diff --git a/Robust.Shared/Utility/ResPath.cs b/Robust.Shared/Utility/ResPath.cs index 91d85cf16..d5a8822ea 100644 --- a/Robust.Shared/Utility/ResPath.cs +++ b/Robust.Shared/Utility/ResPath.cs @@ -66,7 +66,7 @@ public readonly struct ResPath : IEquatable public ResPath(string canonPath) { // Paths should never have non-standardised directory separators passed in, the caller should have already sanitised it. - DebugTools.Assert(!canonPath.Contains('\\')); + DebugTools.Assert(IsValidPath(canonPath)); CanonPath = canonPath; } @@ -77,6 +77,21 @@ public readonly struct ResPath : IEquatable { } + /// + /// Check whether the given string paths contains any non-standard directory separators. + /// + public static bool IsValidPath(string path) => !path.Contains('\\'); + + /// + /// Check whether a string is a valid path () and corresponds to a simple file name. + /// + public static bool IsValidFilename([NotNullWhen(true)] string? filename) + => !string.IsNullOrEmpty(filename) + && IsValidPath(filename) + && !filename.Contains('/') + && filename != "." + && filename != ".."; + /// /// Returns true if the path is equal to "." /// @@ -106,7 +121,7 @@ public readonly struct ResPath : IEquatable } var ind = CanonPath.Length > 1 && CanonPath[^1] == '/' - ? CanonPath[..^1].LastIndexOf('/') + ? CanonPath.LastIndexOf('/', CanonPath.Length - 2) : CanonPath.LastIndexOf('/'); return ind switch { @@ -206,7 +221,7 @@ public readonly struct ResPath : IEquatable // it's a filename // Uses +1 to skip `/` found in or starts from beginning of string // if we found nothing (ind == -1) - var ind = CanonPath[..^1].LastIndexOf('/') + 1; + var ind = CanonPath.LastIndexOf('/', CanonPath.Length - 2) + 1; return CanonPath[^1] == '/' ? CanonPath[ind .. ^1] // Omit last `/` : CanonPath[ind..]; @@ -242,7 +257,6 @@ public readonly struct ResPath : IEquatable return CanonPath.GetHashCode(); } - public static bool operator ==(ResPath left, ResPath right) { return left.Equals(right); @@ -283,7 +297,7 @@ public readonly struct ResPath : IEquatable } // Avoid double separators - if (left.CanonPath.EndsWith("/")) + if (left.CanonPath.EndsWith('/')) { return new ResPath(left.CanonPath + right.CanonPath); } @@ -475,7 +489,7 @@ public readonly struct ResPath : IEquatable /// public string ToRelativeSystemPath() { - return ToRelativePath().ChangeSeparator(SystemSeparatorStr); + return ToRelativePath().ChangeSeparator(SystemSeparator); } /// @@ -495,14 +509,19 @@ public readonly struct ResPath : IEquatable /// public string ChangeSeparator(string newSeparator) { - if (newSeparator is "." or "\0") - { - throw new ArgumentException("New separator can't be `.` or `NULL`"); - } + if (newSeparator.Length != 1) + throw new InvalidOperationException("new separator must be a single character."); + return ChangeSeparator(newSeparator[0]); + } - return newSeparator == "/" - ? CanonPath - : CanonPath.Replace("/", newSeparator); + /// + public string ChangeSeparator(char newSeparator) + { + if (newSeparator is '.' or '\0') + throw new ArgumentException("New separator can't be `.` or `NULL`"); + + // String.Replace() already checks if newSeparator == '/' + return CanonPath.Replace('/', newSeparator); } } diff --git a/Robust.UnitTesting/Shared/Utility/ResPathTest.cs b/Robust.UnitTesting/Shared/Utility/ResPathTest.cs index 62e014ee2..d4a84fedc 100644 --- a/Robust.UnitTesting/Shared/Utility/ResPathTest.cs +++ b/Robust.UnitTesting/Shared/Utility/ResPathTest.cs @@ -36,13 +36,17 @@ public sealed class ResPathTest [TestCase("x/y/z", ExpectedResult = "z")] [TestCase("/bar", ExpectedResult = "bar")] [TestCase("bar/", ExpectedResult = "bar")] // Trailing / gets trimmed. + [TestCase("/foo/bar/", ExpectedResult = "bar")] + // These next two tests are the current behaviour. I don't know if this is how it should behave, these tests just + // ensure that it doesn't change unintentionally + [TestCase("/foo/bar//", ExpectedResult = "")] + [TestCase("/foo/bar///", ExpectedResult = "")] public string FilenameTest(string input) { var resPathFilename = new ResPath(input).Filename; return resPathFilename; } - [Test] [TestCase(@"", ExpectedResult = @".")] [TestCase(@".", ExpectedResult = @".")] @@ -66,6 +70,11 @@ public sealed class ResPathTest [TestCase(@"/foo/bar/x", ExpectedResult = @"/foo/bar")] [TestCase(@"/foo/bar.txt", ExpectedResult = @"/foo")] [TestCase(@"/bar.txt", ExpectedResult = @"/")] + // These next three tests are the current behaviour. I don't know if this is how it should behave, these tests just + // ensure that it doesn't change unintentionally + [TestCase(@"/foo/bar//", ExpectedResult = "/foo/bar")] + [TestCase(@"/foo/bar///", ExpectedResult = "/foo/bar/")] + [TestCase(@"/foo/bar////", ExpectedResult = "/foo/bar//")] public string DirectoryTest(string path) { var resPathDirectory = new ResPath(path).Directory.ToString(); @@ -73,8 +82,7 @@ public sealed class ResPathTest } [Test] - [TestCase(@"a/b/c", "👏", ExpectedResult = "a👏b👏c")] - [TestCase(@"/a/b/c", "👏", ExpectedResult = "👏a👏b👏c")] + [TestCase(@"a/b/c", "\\", ExpectedResult = @"a\b\c")] [TestCase(@"/a/b/c", "\\", ExpectedResult = @"\a\b\c")] public string ChangeSeparatorTest(string input, string separator) {