[GitHub] [commons-io] kinow commented on a change in pull request #192: [IO-707] Add optional postcondition checks with assert

[GitBox <gi...@apache.org>]

kinow commented on a change in pull request #192:
URL: https://github.com/apache/commons-io/pull/192#discussion_r565660147



##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1168,7 +1169,9 @@ static String decodeUrl(final String url) {
      */
     public static File delete(final File file) throws IOException {
         Objects.requireNonNull(file, "file");
-        Files.delete(file.toPath());
+        Path path = file.toPath();
+        Files.delete(path);
+        assert Files.notExists(path, PathUtils.NOFOLLOW_LINK_OPTION_ARRAY) : "File still exists " + path;

Review comment:
       I'm not sure if that's really necessary to have in this method? From the docs for this file, it used to be a useful alternative for `File#delete` that returned a boolean. But now with the JVM (from 8 I think?) `Files#delete` they both have similar behaviour.
   
   So if the file cannot be deleted, an exception is thrown. If the JVM implementation (or underlying OS libraries, or maybe a NFS or virtual file system) reported the file as deleted, but the file still exists, then both `Files#delete` and this method will behave the same way.
   
   I'm not sure if having an assertion error here would be helpful to users of these platforms. They'd probably have to rely on the JVM, or have their code to check for the file still existing (they could be using the JVM `Files#delete` somewhere else in the code).

##########
File path: pom.xml
##########
@@ -394,6 +394,7 @@ file comparators, endian transformation classes, and much more.
           </classpathDependencyExcludes>
           <forkCount>1</forkCount>
           <reuseForks>false</reuseForks>
+          <enableAssertions>true</enableAssertions>

Review comment:
       I haven't used ­or read about assertions in Java in a while. Unless something changed in recent releases, I think this enables the `-ea` flag for build time. But users of the API would still have it disabled by default in OpenJDK/Oracle JVM's.
   
   So the new code with assertions wouldn't be used, and I think using assertions in production code (not in tests) used to be discouraged?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org