dsmiley commented on code in PR #3263:
URL: https://github.com/apache/solr/pull/3263#discussion_r2005759509
##########
solr/core/src/test/org/apache/solr/core/TestLazyCores.java:
##########
@@ -896,7 +896,7 @@ private LocalSolrQueryRequest makeReq(SolrCore core,
String... paramPairs) {
}
private static String makePath(String... args) {
- return String.join(File.separator, args);
+ return String.join(FileSystems.getDefault().getSeparator(), args);
Review Comment:
This isn't wrong but I think Path.of(.....).toString() would be more
idiomatic for its purpose. And looking at it's callers, just 4 right in a row,
you could inline this if you wish (I would).
##########
solr/core/src/test/org/apache/solr/core/TestFileSystemConfigSetService.java:
##########
@@ -73,8 +73,14 @@ public void
testIgnoresFileUploadsOutsideOfConfigSetDirectory() throws IOExcepti
// surface validation errors to enable bulk uploading
final var invalidFilePaths =
List.of(
- ".." + File.separator + "escapePath",
- "foo" + File.separator + ".." + File.separator + ".." +
File.separator + "bar");
+ ".." + FileSystems.getDefault().getSeparator() + "escapePath",
+ "foo"
+ + FileSystems.getDefault().getSeparator()
+ + ".."
+ + FileSystems.getDefault().getSeparator()
+ + ".."
+ + FileSystems.getDefault().getSeparator()
+ + "bar");
for (String invalidFilePath : invalidFilePaths) {
fileSystemConfigSetService.uploadFileToConfig(configName,
invalidFilePath, testdata, true);
Review Comment:
The javadocs of `uploadFileToConfig` (which I recall writing), specify that
the path string provided is forward slash since it's in the abstract namespace
of the config, not on the file system (necessarily).
##########
solr/core/src/test/org/apache/solr/handler/component/FacetPivot2CollectionsTest.java:
##########
@@ -65,8 +65,12 @@ public class FacetPivot2CollectionsTest extends
SolrCloudTestCase {
public static void setupCluster() throws Exception {
// create and configure cluster
configureCluster(1)
- .addConfig(COLL_A, configset("different-stopwords" + File.separator +
COLL_A))
- .addConfig(COLL_B, configset("different-stopwords" + File.separator +
COLL_B))
+ .addConfig(
+ COLL_A,
+ configset("different-stopwords" +
FileSystems.getDefault().getSeparator() + COLL_A))
Review Comment:
I think forward slash always works for Path.resolve, which is what will
happen inside configset method with this string. We'll verify this build on
Windows before merging.
--
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]
