dsmiley commented on code in PR #1239:
URL: https://github.com/apache/solr/pull/1239#discussion_r1063955335
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -259,6 +260,14 @@ public FileVisitResult postVisitDirectory(Path dir,
IOException ioException) {
return filePaths;
}
+ private String normalizePathToForwardSlash(String path) {
+ return path.replace(File.separatorChar, '/');
+ }
+
+ private String normalizePathToOSSeparator(String path) {
Review Comment:
[Google Java Style
guide](https://google.github.io/styleguide/javaguide.html#s5.3-camel-case)
would say "Os" end not "OS" here.
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -259,6 +260,14 @@ public FileVisitResult postVisitDirectory(Path dir,
IOException ioException) {
return filePaths;
}
+ private String normalizePathToForwardSlash(String path) {
+ return path.replace(File.separatorChar, '/');
+ }
+
+ private String normalizePathToOSSeparator(String path) {
+ return path.replace('/', File.separatorChar);
Review Comment:
Instead of referencing the old `java.io.File`, please use
`configSetBase.getFileSystem().getSeparator()`
##########
solr/core/src/test/org/apache/solr/core/TestConfigSetService.java:
##########
@@ -108,16 +110,14 @@ public void testConfigSetServiceOperations() throws
IOException {
assertTrue(configSetService.getConfigMetadata(configName).containsKey("foo"));
List<String> configFiles = configSetService.getAllConfigFiles(configName);
- assertEquals(
- configFiles.toString(),
- "[file1, file2, solrconfig.xml, subdir/, subdir/file3, subdir/file4]");
+ assertEquals(configFiles, List.of(file1, file2, solrConfigXml, subdir,
file3, file4));
Review Comment:
Perfect example of how it was so readable before and now... it's for example
not evident that the output has slashes and what type. Test code is not like
main/prod code; different stylistic choices emphasizing easy readability above
more idealistic decomposition.
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -259,6 +260,14 @@ public FileVisitResult postVisitDirectory(Path dir,
IOException ioException) {
return filePaths;
}
+ private String normalizePathToForwardSlash(String path) {
+ return path.replace(File.separatorChar, '/');
Review Comment:
A short-circuit to do nothing if these are the same; would be nice but don't
have to.
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -250,7 +251,7 @@ public FileVisitResult visitFile(Path file,
BasicFileAttributes attrs)
public FileVisitResult postVisitDirectory(Path dir, IOException
ioException) {
String relativePath = configDir.relativize(dir).toString();
if (!relativePath.isEmpty()) {
- filePaths.add(relativePath + "/");
+ filePaths.add(normalizePathToForwardSlash(relativePath +
File.separator));
Review Comment:
shouldn't this always end in '/' ? If we agree on this, then there's a gap
in the tests.
##########
solr/core/src/test/org/apache/solr/core/TestConfigSetService.java:
##########
@@ -77,24 +72,31 @@ public void testConfigSetServiceOperations() throws
IOException {
byte[] testdata = "test data".getBytes(StandardCharsets.UTF_8);
Path configDir = createTempDir("testconfig");
- Files.createFile(configDir.resolve("solrconfig.xml"));
- Files.write(configDir.resolve("file1"), testdata);
- Files.createFile(configDir.resolve("file2"));
- Files.createDirectory(configDir.resolve("subdir"));
- Files.createFile(configDir.resolve("subdir").resolve("file3"));
+ String solrConfigXml = "solrconfig.xml";
+ Files.createFile(configDir.resolve(solrConfigXml));
+ String file1 = "file1";
+ Files.write(configDir.resolve(file1), testdata);
+ String file2 = "file2";
+ Files.createFile(configDir.resolve(file2));
+ String subDirPath = "subdir";
+ String subdir = subDirPath + "/";
+ Files.createDirectory(configDir.resolve(subDirPath));
+ String file3 = subdir + "file3";
+ Files.createFile(configDir.resolve(subDirPath).resolve("file3"));
+ String file4 = subdir + "file4";
Review Comment:
Ooops.
When I said to keep these changes from your trial PR, I wasn't paying
attention at all. I glanced I guess and thought you were converting from
java.io.File to the Path API -- quite wrong and embarrassing on my part. I
think the test was easier to read before, honestly, because the assertions had
the strings plainly without having to decipher what each variable added means.
**But it's fine; professionals can disagree.**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
