You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "david-patchfox (via GitHub)" <gi...@apache.org> on 2023/10/14 02:40:37 UTC
[PR] adds new method to query the host OS for the temp directory path [commons-io]
david-patchfox opened a new pull request, #500:
URL: https://github.com/apache/commons-io/pull/500
adds new method `FileUtils.getTempDirectoryPathConsistent(boolean)` to give developers the ability to ensure the system temp path is reported in a consistent manner across all OS's.
At issue is the fact that the underlying call to `System.getProperty("java.io.tmpdir")` does not return results in a consistent format. Depending on the host OS, the system file separator may or may not be included at the tail of the result. If the developer is unaware of this behavior their code may break in unexpected ways when ported to an OS that behaves differently than the one they wrote the code on. See these bug reports for details and background:
https://bugs.java.com/bugdatabase/view_bug?bug_id=4391434
https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8318067
PR does not change behavior of existing `FileUtils.getTempDirectoryPath()` but does add a docstring comment alerting developers of the issue and directing them to `FileUtils.getTempDirectoryPathConsistent(boolean)` if need to ensure the system temp directory is reported in a consistent manner across all host OS's.
New method is tested by way of `FileUtilsTest.testGetTempDirectoryPathConsistentNoTrailingSeparator()` and `FileUtilesTest.testGetTempDirectoryPathConsistentWithTrailingSeparator()`.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "sebbASF (via GitHub)" <gi...@apache.org>.
sebbASF commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364653992
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
+ String systemFileSeparator = System.getProperty("file.separator");
+ String lastCharOfTempDirectoryPath = tempDirectoryPath.substring(tempDirectoryPath.length() - 1);
+ assertNotEquals(lastCharOfTempDirectoryPath, systemFileSeparator);
+ }
+
+ @Test
+ public void testGetTempDirectoryPathConsistentWithTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(true);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
Review Comment:
That the method does not generate the correct result at some time in the future due to code changes.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769554831
> I agree that it would be helpful if existing method Javadoc noted that the return string may or may not include a trailing separator.
>
> However, adding more code and test cases (which have to be maintained) seems completely unnecessary to me. Indeed counter-productive, as it encourages string manipulation rather than proper use of the appropriate routines.
>
> I think we should just update the Javadoc and suggest coders use the proper methods.
Respectfully - y'all seem fixated on telling people what "proper" is and as tool makers I think that's wrong headed. Not everyone is going to limit themselves in how they use what you've made to the limit of your imagination of how things can or should be done. It's a big world and none of us - not y'all and not me - can possibly enumerate every permutation of every circumstance in which a tool needs to be employed. In fact - I would argue exploring the delta between what was intended and what's possible is the fun and delight- and dare I say the point - of coding.
That being said - this is your tool and not mine. We had a problem and fixed it on our end and I put in a PR to be a mensch to the community by sharing that additional logic. Take it or leave it. I'm happy to continue putting in PR changes as per request but I have to say I'm more than a little surprised at the level of passionate pushback y'all have given me to the idea that someone might (gasp) be using public methods in your API that have been there for years in the manner in which the documentation says is intended.
:beers: friends
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364619659
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
+ String systemFileSeparator = System.getProperty("file.separator");
+ String lastCharOfTempDirectoryPath = tempDirectoryPath.substring(tempDirectoryPath.length() - 1);
+ assertNotEquals(lastCharOfTempDirectoryPath, systemFileSeparator);
Review Comment:
huh? the method being tested is pulling the system temp directory in exactly the same way as the existing `FileUtils.getTempDirectoryPath()` method and that's by calling `System.getProperty("java.io.tmpdir")` which gets populated during phase1init by way of SystemProps.java ([link](https://github.com/openjdk/jdk/blob/e25a49a993f270c33f7929e629fb3075a11fdec9/src/java.base/share/classes/jdk/internal/util/SystemProps.java#L113C72-L113C72)) .
what's the reasonable concern here?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "sebbASF (via GitHub)" <gi...@apache.org>.
sebbASF commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1768971267
Further quick thought: when using the value from getTempDirectoryPath, if this is passed to the appropriate path joining methods, won't they handle the trailing character?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769042382
Path methods handle separators, so one should use those APIs these days...
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364619901
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
+ String systemFileSeparator = System.getProperty("file.separator");
+ String lastCharOfTempDirectoryPath = tempDirectoryPath.substring(tempDirectoryPath.length() - 1);
+ assertNotEquals(lastCharOfTempDirectoryPath, systemFileSeparator);
+ }
+
+ @Test
+ public void testGetTempDirectoryPathConsistentWithTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(true);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
Review Comment:
same as above - what's the reasonable concern here?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "sebbASF (via GitHub)" <gi...@apache.org>.
sebbASF commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364218176
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1444,7 +1444,7 @@ private static File getParentFile(final File file) {
/**
* Returns a {@link File} representing the system temporary directory.
- *
+ *
Review Comment:
Spurious change; please remove from PR
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1453,8 +1453,38 @@ public static File getTempDirectory() {
}
/**
- * Returns the path to the system temporary directory.
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
+ * file separator.
*
+ * @param includeTrailingSeparator switch indicating whether or not you want the returned path to include a
+ * trailing file separator.
+ * @return the path to the system temporary directory with or without the trailing file separator as per
+ * caller supplied argument.
+ * @since 2.14.1
+ */
+ public static String getTempDirectoryPathConsistent(boolean includeTrailingSeparator) {
+ String tempDir = System.getProperty("java.io.tmpdir");
+ String tempDirLastChar = tempDir.substring(tempDir.length() - 1);
+ String systemFileSeparator = System.getProperty("file.separator");
+ if (includeTrailingSeparator) {
+ tempDir = tempDirLastChar.equals(systemFileSeparator)
+ ? tempDir
+ : tempDir + systemFileSeparator;
+ } else {
+ tempDir = tempDirLastChar.equals(systemFileSeparator)
+ ? tempDir.substring(0, tempDir.length() - 1)
+ : tempDir;
+ }
+
+ return tempDir;
+ }
+
+ /**
+ * Returns the path to the system temporary directory.
+ *
+ * @apiNote this method relies on Java system property 'java.io.tmpdir' which may or may not return a path with
+ * a trailing file separator, depending on the OS Java is running in. If you need the temp directory reported in a
Review Comment:
java.io.tmpdir can also be set by the user, so it is not strictly correct to say that the trailing character depends on the OS that Java is using. It would be more accurate to say that it depends on the OS environment.
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1587,13 +1587,29 @@ public void testGetFile_Parent() {
public void testGetTempDirectory() {
final File tempDirectory = new File(FileUtils.getTempDirectoryPath());
assertEquals(tempDirectory, FileUtils.getTempDirectory());
- }
+ }
Review Comment:
Trailing space
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "robtimus (via GitHub)" <gi...@apache.org>.
robtimus commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364355936
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1452,9 +1452,39 @@ public static File getTempDirectory() {
return new File(getTempDirectoryPath());
}
+ /**
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
+ * file separator.
+ *
+ * @param includeTrailingSeparator switch indicating whether or not you want the returned path to include a
+ * trailing file separator.
+ * @return the path to the system temporary directory with or without the trailing file separator as per
+ * caller supplied argument.
+ * @since 2.14.1
+ */
+ public static String getTempDirectoryPathConsistent(boolean includeTrailingSeparator) {
+ String tempDir = System.getProperty("java.io.tmpdir");
+ String tempDirLastChar = tempDir.substring(tempDir.length() - 1);
+ String systemFileSeparator = System.getProperty("file.separator");
Review Comment:
Just a thought, but is it possible to use `tempDir.charAt(tempDir.length() - 1)` and `File.separatorChar`?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "sebbASF (via GitHub)" <gi...@apache.org>.
sebbASF commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1770546193
Note: it looks like you could just use FileUtils.getTempDirectory().toString() instead.
AFAICT that drops the trailing separator.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364435292
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,22 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
Review Comment:
done and done. I've added assertions to both tests that ensure the root path is the same as is reported by property `java.io.tmpdir`
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364358453
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1452,9 +1452,39 @@ public static File getTempDirectory() {
return new File(getTempDirectoryPath());
}
+ /**
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
+ * file separator.
+ *
+ * @param includeTrailingSeparator switch indicating whether or not you want the returned path to include a
+ * trailing file separator.
+ * @return the path to the system temporary directory with or without the trailing file separator as per
+ * caller supplied argument.
+ * @since 2.14.1
+ */
+ public static String getTempDirectoryPathConsistent(boolean includeTrailingSeparator) {
+ String tempDir = System.getProperty("java.io.tmpdir");
+ String tempDirLastChar = tempDir.substring(tempDir.length() - 1);
+ String systemFileSeparator = System.getProperty("file.separator");
Review Comment:
what do you mean? you want ln1468 changed to use `File.separatorChar` ? what would be the difference?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox closed pull request #500: adds new method to query the host OS for the temp directory path
URL: https://github.com/apache/commons-io/pull/500
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364372114
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1452,9 +1452,39 @@ public static File getTempDirectory() {
return new File(getTempDirectoryPath());
}
+ /**
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
+ * file separator.
+ *
+ * @param includeTrailingSeparator switch indicating whether or not you want the returned path to include a
+ * trailing file separator.
+ * @return the path to the system temporary directory with or without the trailing file separator as per
+ * caller supplied argument.
+ * @since 2.14.1
+ */
+ public static String getTempDirectoryPathConsistent(boolean includeTrailingSeparator) {
+ String tempDir = System.getProperty("java.io.tmpdir");
+ String tempDirLastChar = tempDir.substring(tempDir.length() - 1);
+ String systemFileSeparator = System.getProperty("file.separator");
Review Comment:
> what do you mean? you want ln1468 changed to use `File.separatorChar` ? what would be the difference?
It's better to use an API than a magic string.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364419330
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,22 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
Review Comment:
eh ok. I think that's overkill but I can change that. wait one and I'll push an update to the test
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "sebbASF (via GitHub)" <gi...@apache.org>.
sebbASF commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364538190
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1452,9 +1452,39 @@ public static File getTempDirectory() {
return new File(getTempDirectoryPath());
}
+ /**
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
Review Comment:
disinclude => exclude
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
Review Comment:
If tmpdir = '/tmp/a/' then surely tempDirectoryPath => '/tmp/a' and the assertion fails?
Maybe the arguments should be the other way round?
Also if tempDirectoryPath is mis-calculated as '/tmp/a/b/c/d', then the assertion does not fail.
tmpdir.length() - tempDirectoryPath.length() should be 0 or 1
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
+ String systemFileSeparator = System.getProperty("file.separator");
+ String lastCharOfTempDirectoryPath = tempDirectoryPath.substring(tempDirectoryPath.length() - 1);
+ assertNotEquals(lastCharOfTempDirectoryPath, systemFileSeparator);
Review Comment:
This assumes that tmpdir cannot end with two consecutive separators
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
+ String systemFileSeparator = System.getProperty("file.separator");
+ String lastCharOfTempDirectoryPath = tempDirectoryPath.substring(tempDirectoryPath.length() - 1);
+ assertNotEquals(lastCharOfTempDirectoryPath, systemFileSeparator);
+ }
+
+ @Test
+ public void testGetTempDirectoryPathConsistentWithTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(true);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
Review Comment:
Does not allow for spurious trailing content.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769516166
> I agree that it would be helpful if existing method Javadoc noted that the return string may or may not include a trailing separator.
>
> However, adding more code and test cases (which have to be maintained) seems completely unnecessary to me. Indeed counter-productive, as it encourages string manipulation rather than proper use of the appropriate routines.
>
> I think we should just update the Javadoc and suggest coders use the proper methods.
I agree wholeheartedly with @sebbASF, I am -1 to this PR as it stands.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769102067
> Further quick thought: when using the value from getTempDirectoryPath, if this is passed to the appropriate path joining methods, won't they handle the trailing character?
maybe but the caller may be doing something where path joining doesn't make sense and/or would create unnecessary code bloat. if the API is going to offer a public method that returns the temp directory as a String then - IMHO - it needs to document the unpredictability of the response and offer an alternative that permits the caller to both ensure consistency and control what that consistent behavior looks like.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "sebbASF (via GitHub)" <gi...@apache.org>.
sebbASF commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364653992
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
+ String systemFileSeparator = System.getProperty("file.separator");
+ String lastCharOfTempDirectoryPath = tempDirectoryPath.substring(tempDirectoryPath.length() - 1);
+ assertNotEquals(lastCharOfTempDirectoryPath, systemFileSeparator);
+ }
+
+ @Test
+ public void testGetTempDirectoryPathConsistentWithTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(true);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
Review Comment:
That the method does not generate the correct result at some time in the future due to code changes.
When writing these tests, they have to be done without assuming any knowledge of how the code is written.
So one cannot assume that the only error might be incorrect treatment of a trailing separator.
You have to assume that the code might generate arbitrary output.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364650861
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1452,9 +1452,39 @@ public static File getTempDirectory() {
return new File(getTempDirectoryPath());
}
+ /**
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
Review Comment:
done
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769260972
> I still don't see why this is needed. Just call: `org.apache.commons.io.file.PathUtils.getTempDirectory()`.
actually one more thing as I'm getting the impression you still aren't fathoming why someone would do something differently than you think they should - here is an example:
the Path API doesn't permit you to split in the same way a String does. The system tmp folder is named differently depending on what OS the JVM is running on and is therefore of variable path length (macos in particular). If you are doing ETL work with archived input you need to know where certain relative paths are indexed against system tmp it takes several extra statements of code to do that the Path way vs splitting a path string by whatever `java.io.tmpdir` reports as then using the pre-determined relative indexing the ETL packaging scheme is using. Java is already a highly verbose language and not everyone enjoys making a factory factory for the session helper manager to generation a session manager helper to do what ostensibly are basic tasks.
In other words - sometimes some people have a good reason to want to use Strings and not Path and that's ok :smiley:
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769247246
> I still don't see why this is needed. Just call: `org.apache.commons.io.file.PathUtils.getTempDirectory()`.
Bottom line - y'all expose a public method to return this as a String and there's presently nothing in the API doc warning developers of the inconsistent nature of how the underlying java system property will report the temp directory in that circumstance. This does that and provides an alternative method for folks - like us - don't want to diddle with Path objects because doing it the Path way creates bloat code that doesn't need to be there.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1768970517
## [Codecov](https://app.codecov.io/gh/apache/commons-io/pull/500?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#500](https://app.codecov.io/gh/apache/commons-io/pull/500?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (580747a) into [master](https://app.codecov.io/gh/apache/commons-io/commit/7ec9b573c4a9503a8b3673146455a1369cebcd0b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7ec9b57) will **decrease** coverage by `0.01%`.
> Report is 6 commits behind head on master.
> The diff coverage is `75.00%`.
```diff
@@ Coverage Diff @@
## master #500 +/- ##
============================================
- Coverage 84.89% 84.88% -0.01%
- Complexity 3382 3384 +2
============================================
Files 228 228
Lines 8116 8124 +8
Branches 961 964 +3
============================================
+ Hits 6890 6896 +6
Misses 963 963
- Partials 263 265 +2
```
| [Files](https://app.codecov.io/gh/apache/commons-io/pull/500?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [src/main/java/org/apache/commons/io/FileUtils.java](https://app.codecov.io/gh/apache/commons-io/pull/500?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vRmlsZVV0aWxzLmphdmE=) | `94.41% <75.00%> (-0.29%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364384993
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,22 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
Review Comment:
This test is not valid: It does not assert the actual path is correct. For example, if the new API returns a "C:\Some\Odd\Dir" that is not the Java temp dir, then the test would pass.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "sebbASF (via GitHub)" <gi...@apache.org>.
sebbASF commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769371210
I agree that it would be helpful if existing method Javadoc noted that the return string may or may not include a trailing separator.
However, adding more code and test cases (which have to be maintained) seems completely unnecessary to me.
Indeed counter-productive, as it encourages string manipulation rather than proper use of the appropriate routines.
I think we should just update the Javadoc and suggest coders use the proper methods.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769848251
> > I agree that it would be helpful if existing method Javadoc noted that the return string may or may not include a trailing separator.
> > However, adding more code and test cases (which have to be maintained) seems completely unnecessary to me. Indeed counter-productive, as it encourages string manipulation rather than proper use of the appropriate routines.
> > I think we should just update the Javadoc and suggest coders use the proper methods.
>
> I agree wholeheartedly with @sebbASF, I am -1 to this PR as it stand
> > I agree that it would be helpful if existing method Javadoc noted that the return string may or may not include a trailing separator.
> > However, adding more code and test cases (which have to be maintained) seems completely unnecessary to me. Indeed counter-productive, as it encourages string manipulation rather than proper use of the appropriate routines.
> > I think we should just update the Javadoc and suggest coders use the proper methods.
>
> I agree wholeheartedly with @sebbASF, I am -1 to this PR as it stands.
ok cool. :beers:
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "robtimus (via GitHub)" <gi...@apache.org>.
robtimus commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364366688
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1452,9 +1452,39 @@ public static File getTempDirectory() {
return new File(getTempDirectoryPath());
}
+ /**
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
+ * file separator.
+ *
+ * @param includeTrailingSeparator switch indicating whether or not you want the returned path to include a
+ * trailing file separator.
+ * @return the path to the system temporary directory with or without the trailing file separator as per
+ * caller supplied argument.
+ * @since 2.14.1
+ */
+ public static String getTempDirectoryPathConsistent(boolean includeTrailingSeparator) {
+ String tempDir = System.getProperty("java.io.tmpdir");
+ String tempDirLastChar = tempDir.substring(tempDir.length() - 1);
+ String systemFileSeparator = System.getProperty("file.separator");
Review Comment:
It would use `char` instead of `String` for the comparison, which would save some memory. Perhaps it's premature optimization, but whenever I can use primitives instead of objects I go for it.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364374839
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -1452,9 +1452,39 @@ public static File getTempDirectory() {
return new File(getTempDirectoryPath());
}
+ /**
+ * Returns the path to the system temporary directory with the option to include or disinclude the trailing
+ * file separator.
+ *
+ * @param includeTrailingSeparator switch indicating whether or not you want the returned path to include a
+ * trailing file separator.
+ * @return the path to the system temporary directory with or without the trailing file separator as per
+ * caller supplied argument.
+ * @since 2.14.1
+ */
+ public static String getTempDirectoryPathConsistent(boolean includeTrailingSeparator) {
+ String tempDir = System.getProperty("java.io.tmpdir");
+ String tempDirLastChar = tempDir.substring(tempDir.length() - 1);
+ String systemFileSeparator = System.getProperty("file.separator");
Review Comment:
I agree - that feels like a premature optimization :smile:
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364419330
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,22 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
Review Comment:
eh ok. I think that's overkill but I can change that. the method being tested is a wrapper for a different method that is already covered by the test you're looking for. that being said - wait one and I'll push an update to the test
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #500:
URL: https://github.com/apache/commons-io/pull/500#issuecomment-1769167789
I still don't see why this is needed. Just call: `org.apache.commons.io.file.PathUtils.getTempDirectory()`.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds new method to query the host OS for the temp directory path [commons-io]
Posted by "david-patchfox (via GitHub)" <gi...@apache.org>.
david-patchfox commented on code in PR #500:
URL: https://github.com/apache/commons-io/pull/500#discussion_r1364619659
##########
src/test/java/org/apache/commons/io/FileUtilsTest.java:
##########
@@ -1594,6 +1594,24 @@ public void testGetTempDirectoryPath() {
assertEquals(System.getProperty("java.io.tmpdir"), FileUtils.getTempDirectoryPath());
}
+ @Test
+ public void testGetTempDirectoryPathConsistentNoTrailingSeparator() {
+ String tempDirectoryPath = FileUtils.getTempDirectoryPathConsistent(false);
+ assertTrue(tempDirectoryPath.startsWith(System.getProperty("java.io.tmpdir")));
+ String systemFileSeparator = System.getProperty("file.separator");
+ String lastCharOfTempDirectoryPath = tempDirectoryPath.substring(tempDirectoryPath.length() - 1);
+ assertNotEquals(lastCharOfTempDirectoryPath, systemFileSeparator);
Review Comment:
huh? the method being tested is pulling the system temp directory in exactly the same way as the existing `FileUtils.getTempDirectoryPath()` method and that's by calling `System.getProperty("java.io.tmpdir")` which gets populated during phase1init by way of SystemProps.java ([link](https://github.com/openjdk/jdk/blob/e25a49a993f270c33f7929e629fb3075a11fdec9/src/java.base/share/classes/jdk/internal/util/SystemProps.java#L113C72-L113C72)) .
what's the reasonable concern here?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org