[GitHub] [maven-shared-utils] elharo commented on a change in pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs

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

elharo commented on a change in pull request #41:
URL: https://github.com/apache/maven-shared-utils/pull/41#discussion_r432418893



##########
File path: src/main/java/org/apache/maven/shared/utils/PropertyUtils.java
##########
@@ -89,22 +89,21 @@ public static Properties loadProperties( @Nonnull File file )
 
     /**
      * @param is {@link InputStream}
-     * @return The loaded properties.
-     * @deprecated As of 3.1.0, please use method {@link #loadOptionalProperties(java.io.InputStream)}. This method
+     * @return the loaded properties
+     * @deprecated use {@link #loadOptionalProperties(java.io.InputStream)} instead. This method
      *             should not be used as it suppresses exceptions silently when loading properties fails.
      */
     @Deprecated
     public static Properties loadProperties( @Nullable InputStream is )
     {
         try
         {
-            // to make this the same behaviour as the others we should really return null on any error
             Properties result = new Properties();
             if ( is != null )
             {
-                try
+                try ( InputStream in = is )

Review comment:
       That makes sense. It is a behavior change though. Previously the stream was closed  in lines 120-123. This PR just changes how it's closed to avoid a now deprecated class. If we stop closing it here (and in other methods) then that might be worth a separate bug and PR that shows up in the release notes. What do you think? 




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