Merge pull request #3634 from brianlheim/topic/path-append

classlib: fix String methods that use path separators
supercollider · Apr 19, 2018 · 4880b66 · 4880b66
2 parents 0e5c36b + d0ead69
commit 4880b66
Show file tree

Hide file tree

Showing 3 changed files with 148 additions and 20 deletions.
diff --git a/HelpSource/Classes/String.schelp b/HelpSource/Classes/String.schelp
@@ -277,8 +277,32 @@ code::
 ::
 
 method::+/+
-Path concatenation operator - useful for avoiding doubling-up slashes unnecessarily.
-code::"foo"+/+"bar":: returns code::"foo/bar"::
+Concatenates code::this:: and code::path::, as components of a filesystem path on the host operating
+system. The strings are joined to avoid duplicate path separators.
+
+If code::this:: ends with a path separator and code::path:: begins with one, then the separator
+in code::path:: is dropped. If there is a path separator on either side, this has the same effect as
+using code::++::. If neither side has a path separator, the platform's preferred separator
+(code::\\:: on Windows, code::/:: otherwise) is added.
+
+If code::path:: is a link::Classes/PathName::, the result will be a PathName.
+
+argument::path
+
+Either a String or link::Classes/PathName::.
+
+code::
+// On Windows, this produces "foo\\\\bar"; on other platforms, "foo/bar"
+"foo" +/+ "bar"
+
+// On all platforms, this produces "foo/bar": +/+ prefers using an existing separator
+"foo/" +/+ "bar"
+"foo" +/+ "/bar"
+"foo/" +/+ "/bar"
+
+// On Windows, this produces "foo\\\\bar"; on other platforms, "foo/\\\\bar"
+"foo" +/+ "\\\\bar"
+::
 
 method::catArgs
 Concatenate this string with the following args.
@@ -710,6 +734,10 @@ subsection::Pathname Support
 
 Also see link::#-+/+:: for path concatenation.
 
+The term "path separator" is a platform-independent term for the character(s) that can be used to
+separate components of a path. On Windows, both forward slash "/" and backward slash "\\" are path
+separators. On POSIX-based systems like macOS and Linux, only forward slash is allowed.
+
 method::shellQuote
 Return a new string suitable for use as a filename in a shell command, by enclosing it in single quotes ( teletype::':: ).
 If the string contains any single quotes they will be escaped.
@@ -733,10 +761,10 @@ argument::relativeTo
 The path to make this path relative to.
 
 method::withTrailingSlash
-Return this string with a trailing slash if that was not already the case.
+Appends a path separator if one is not already present.
 
 method::withoutTrailingSlash
-Return this string without a trailing slash if that was not already the case.
+Removes a trailing path separator if one is present.
 
 method::basename
 Return the filename from a Unix path.

diff --git a/SCClassLibrary/Common/Collections/String.sc b/SCClassLibrary/Common/Collections/String.sc
@@ -343,7 +343,7 @@ String[char] : RawArray {
 
 	inspectorClass { ^StringInspector }
 
-	/// unix
+	// -------- path operations --------------------------------------------------
 
 	standardizePath {
 		_String_StandardizePath
@@ -353,21 +353,21 @@ String[char] : RawArray {
 		_String_RealPath
 		^this.primitiveFailed
 	}
+
 	withTrailingSlash {
-		var sep = thisProcess.platform.pathSeparator;
-		if(this.last != sep, {
-			^this ++ sep
-		},{
-			^this
-		})
+		^if(this.isEmpty or: { this.last.isPathSeparator.not }) {
+			this ++ thisProcess.platform.pathSeparator
+		} {
+			this
+		}
 	}
+
 	withoutTrailingSlash {
-		var sep = thisProcess.platform.pathSeparator;
-		if(this.last == sep,{
-			^this.copyRange(0, this.size-2)
-		},{
-			^this
-		})
+		^if(this.isEmpty or: { this.last.isPathSeparator.not }) {
+			this
+		} {
+			this.drop(-1)
+		}
 	}
 
 	absolutePath {
@@ -442,17 +442,25 @@ String[char] : RawArray {
 
 	// path concatenate
 	+/+ { arg path;
-		var pathSeparator = thisProcess.platform.pathSeparator;
+		var sep = thisProcess.platform.pathSeparator;
+		var hasLeftSep, hasRightSep;
 
 		if (path.respondsTo(\fullPath)) {
 			^PathName(this +/+ path.fullPath)
 		};
 
-		if (this.last == pathSeparator or: { path.first == pathSeparator }) {
+		hasLeftSep = this.notEmpty and: { this.last.isPathSeparator };
+		hasRightSep = path.notEmpty and: { path.first.isPathSeparator };
+		if(hasLeftSep && hasRightSep) {
+			// prefer using the LHS separator
+			^this ++ path.drop(1)
+		};
+
+		if(hasLeftSep || hasRightSep) {
 			^this ++ path
 		};
 
-		^this ++ pathSeparator ++ path
+		^this ++ sep ++ path
 	}
 
 	asRelativePath { |relativeTo|

diff --git a/testsuite/classlibrary/TestString.sc b/testsuite/classlibrary/TestString.sc
@@ -0,0 +1,92 @@
+TestString : UnitTest {
+
+	// ------- path-like operations ---------------------------------------------
+
+	test_withTrailingSlash_onEmptyString_addsSeparator {
+		var expected = thisProcess.platform.pathSeparator.asString;
+		this.assertEquals("".withTrailingSlash, expected);
+	}
+
+	test_withTrailingSlash_onPathSeparator_isNoop {
+		var sep = thisProcess.platform.pathSeparator.asString;
+		this.assertEquals(sep.withTrailingSlash, sep);
+	}
+
+	test_withoutTrailingSlash_onEmptyString_isNoop {
+		this.assertEquals("".withoutTrailingSlash, "");
+	}
+
+	test_withoutTrailingSlash_onSomeString_isNoop {
+		var str = "hello";
+		this.assertEquals(str.withoutTrailingSlash, str);
+	}
+
+	test_withoutTrailingSlash_onSeparator_removesSep {
+		var sep = thisProcess.platform.pathSeparator.asString;
+		this.assertEquals(sep.withoutTrailingSlash, "");
+	}
+
+	// Windows should treat slash as a path sep
+	test_withoutTrailingSlash_onSlash_removesSep {
+		this.assertEquals("/".withoutTrailingSlash, "");
+	}
+
+	// operator +/+
+	test_appendPathSep_emptyWithEmpty_producesSep {
+		var sep = thisProcess.platform.pathSeparator.asString;
+		this.assertEquals("" +/+ "", sep);
+	}
+
+	test_appendPathSep_nonSepWithNonSep_producesSep {
+		var sep = thisProcess.platform.pathSeparator.asString;
+		this.assertEquals("abc" +/+ "def", "abc" ++ sep ++ "def");
+	}
+
+	test_appendPathSep_sepWithNonSep_onlyOneSep {
+		var sep = thisProcess.platform.pathSeparator.asString;
+		var result = ("abc" ++ sep) +/+ ("def");
+		var expected = "abc" ++ sep ++ "def";
+		this.assertEquals(result, expected);
+	}
+
+	test_appendPathSep_nonSepWithSep_onlyOneSep {
+		var sep = thisProcess.platform.pathSeparator.asString;
+		var result = ("abc") +/+ (sep ++ "def");
+		var expected = "abc" ++ sep ++ "def";
+		this.assertEquals(result, expected);
+	}
+
+	test_appendPathSep_sepWithSep_onlyOneSep {
+		var sep = thisProcess.platform.pathSeparator.asString;
+		var result = ("abc" ++ sep) +/+ (sep ++ "def");
+		var expected = "abc" ++ sep ++ "def";
+		this.assertEquals(result, expected);
+	}
+
+	// Windows should accept / as a path separator in these cases, and prefer using the LHS slash
+	test_appendPathSep_slashWithSlash_onlyOneSep {
+		var result = "abc/" +/+ "/def";
+		var expected = "abc/def";
+		this.assertEquals(result, expected);
+	}
+
+	test_appendPathSep_slashWithBackslash_onlyOneSep {
+		var result = "abc/" +/+ "\\def";
+		var expected = thisProcess.platform.isPathSeparator($\\).if { "abc/def" } { "abc/\\def" };
+		this.assertEquals(result, expected);
+	}
+
+	test_appendPathSep_backslashWithBackslash {
+		var result = "abc\\" +/+ "\\def";
+		var expected = thisProcess.platform.isPathSeparator($\\).if { "abc\\def" } { "abc\\/\\def" };
+		this.assertEquals(result, expected);
+	}
+
+	test_appendPathSep_stringWithPathName_convertsToPathName {
+		var result = "abc" +/+ PathName("def");
+		var expected = PathName("abc" +/+ "def");
+		this.assertEquals(result.fullPath, expected.fullPath);
+	}
+
+}
+