You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/01/28 17:14:02 UTC

[GitHub] [commons-io] boris-unckel opened a new pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

boris-unckel opened a new pull request #196:
URL: https://github.com/apache/commons-io/pull/196


   Fixes https://issues.apache.org/jira/browse/IO-711


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

[GitHub] [commons-io] boris-unckel commented on pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
boris-unckel commented on pull request #196:
URL: https://github.com/apache/commons-io/pull/196#issuecomment-769871226


   @garydgregory 
   - Fixed the formatting (is there a official eclipse configuration you can recommend?)
   - Removed single parameter checks in case of direct usage, 
   - Kept single parameter checks when prior (different) Object creation happens (see case above)
   - Kept multiple parameter checks
   - Removed double checks in constructors, even if objects are created
   - Kept all checks in constructors, where fail will be in post constructor usage of 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.

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

[GitHub] [commons-io] coveralls commented on pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #196:
URL: https://github.com/apache/commons-io/pull/196#issuecomment-770146131


   
   [![Coverage Status](https://coveralls.io/builds/36715560/badge)](https://coveralls.io/builds/36715560)
   
   Coverage increased (+0.1%) to 89.121% when pulling **cb1751782b038c10b4219fa5208cfeccfc627634 on boris-unckel:utilize_objects_requirenotnull** into **8665a70d1ea00939d12c7a22ff6c8a8124924acd on apache:master**.
   


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

[GitHub] [commons-io] garydgregory commented on pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #196:
URL: https://github.com/apache/commons-io/pull/196#issuecomment-855929320


   @boris-unckel 
   May you please rebase on master?


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

[GitHub] [commons-io] garydgregory commented on pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #196:
URL: https://github.com/apache/commons-io/pull/196#issuecomment-769397122


   > @garydgregory Please review and mark the cases you don't want to change for compatibility and/or design reasons. I'll change then all occurrences accordingly.
   
   Hi @boris-unckel 
   
   Sorry but I don't want to take the time to review each change in each of 31 files. If you do not want to revise the PR to minimize changes and duplication of null checks, that's fine as well. I might get to these kinds of changes later. Thank you for your contributions so far though :-)


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

[GitHub] [commons-io] boris-unckel commented on pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
boris-unckel commented on pull request #196:
URL: https://github.com/apache/commons-io/pull/196#issuecomment-769239872


   @garydgregory Please review and mark the cases you don't want to change for compatibility and/or design reasons. I'll change then all occurrences accordingly.


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

[GitHub] [commons-io] boris-unckel closed pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
boris-unckel closed pull request #196:
URL: https://github.com/apache/commons-io/pull/196


   


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

[GitHub] [commons-io] garydgregory commented on a change in pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #196:
URL: https://github.com/apache/commons-io/pull/196#discussion_r566371592



##########
File path: src/main/java/org/apache/commons/io/FileDeleteStrategy.java
##########
@@ -144,6 +147,7 @@ public boolean deleteQuietly(final File fileToDelete) {
      * @throws IOException if an error occurs during file deletion
      */
     protected boolean doDelete(final File file) throws IOException {
+        Objects.requireNonNull(file,"file");

Review comment:
       Fix formatting.
   

##########
File path: src/main/java/org/apache/commons/io/FileSystemUtils.java
##########
@@ -271,9 +272,8 @@ public static long freeSpaceKb(final long timeout) throws IOException {
      * @throws IOException if an error occurs when finding the free space
      */
     long freeSpaceOS(final String path, final int os, final boolean kb, final long timeout) throws IOException {
-        if (path == null) {
-            throw new IllegalArgumentException("Path must not be null");
-        }
+        Objects.requireNonNull(path,"path");

Review comment:
       Fix formatting.

##########
File path: src/main/java/org/apache/commons/io/FileSystemUtils.java
##########
@@ -396,6 +396,7 @@ long parseDir(final String line, final String path) throws IOException {
      */
     long freeSpaceUnix(final String path, final boolean kb, final boolean posix, final long timeout)
             throws IOException {
+        Objects.requireNonNull(path,"path");

Review comment:
       Fix formatting.

##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -340,6 +341,7 @@ public static void cleanDirectory(final File directory) throws IOException {
      * @see #forceDeleteOnExit(File)
      */
     private static void cleanDirectoryOnExit(final File directory) throws IOException {
+        Objects.requireNonNull(directory, "directory");

Review comment:
       -1: no need, already done on the next line in `listFiles`.

##########
File path: src/main/java/org/apache/commons/io/FilenameUtils.java
##########
@@ -1510,6 +1510,9 @@ public static boolean wildcardMatch(final String fileName, final String wildcard
      * @return true if the given name is a valid host name
      */
     private static boolean isValidHostName(final String name) {
+        if(Objects.isNull(name)) {

Review comment:
       Fix formatting and no need to call an API meant for use in lambdas when `name == null` would do.
   

##########
File path: src/main/java/org/apache/commons/io/monitor/FileAlterationObserver.java
##########
@@ -200,12 +201,9 @@ public FileAlterationObserver(final File directory, final FileFilter fileFilter,
      */
     protected FileAlterationObserver(final FileEntry rootEntry, final FileFilter fileFilter,
                                      final IOCase caseSensitivity) {
-        if (rootEntry == null) {
-            throw new IllegalArgumentException("Root entry is missing");
-        }
-        if (rootEntry.getFile() == null) {
-            throw new IllegalArgumentException("Root directory is missing");
-        }
+        Objects.requireNonNull(rootEntry, "rootEntry");
+        Objects.requireNonNull(rootEntry.getFile(),"rootDirectory");

Review comment:
       Fix formatting.
   

##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -866,6 +868,8 @@ public static void copyFile(final File srcFile, final File destFile, final CopyO
      * @since 2.1
      */
     public static long copyFile(final File input, final OutputStream output) throws IOException {
+        Objects.requireNonNull(input, "input");
+        Objects.requireNonNull(output, "output");

Review comment:
       -1: no need to check on `output`, this already done in `copyLarge`.
   
   I'll stop this type of comment since these checks over the top in this PR. IOW, let's not check for NPEs when we do not need to.
   

##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -314,6 +314,7 @@ public static long checksumCRC32(final File file) throws IOException {
      * @see #forceDelete(File)
      */
     public static void cleanDirectory(final File directory) throws IOException {
+        Objects.requireNonNull(directory, "directory");

Review comment:
       -1: no need, already done on the next line in `listFiles`.

##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1200,6 +1210,7 @@ public static void deleteDirectory(final File directory) throws IOException {
      * @throws IOException          in case deletion is unsuccessful
      */
     private static void deleteDirectoryOnExit(final File directory) throws IOException {
+        Objects.requireNonNull(directory,"directory");

Review comment:
       Fix formatting.




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

[GitHub] [commons-io] boris-unckel commented on a change in pull request #196: [IO-711] Use Objects.requireNotNull for fail fast method/constructors

Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #196:
URL: https://github.com/apache/commons-io/pull/196#discussion_r566852479



##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -866,6 +868,8 @@ public static void copyFile(final File srcFile, final File destFile, final CopyO
      * @since 2.1
      */
     public static long copyFile(final File input, final OutputStream output) throws IOException {
+        Objects.requireNonNull(input, "input");
+        Objects.requireNonNull(output, "output");

Review comment:
       @garydgregory The other cases before I understood the pattern behind "-1". In this case a FileInputStream is created, afterwards copyLarge calls copy calls copyLarge which then does the check.
   I'll fix the former cases and wait with removal for cases like this on your comment.




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