You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/05/29 10:42:31 UTC
[GitHub] [maven-shared-utils] elharo opened a new pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs
elharo opened a new pull request #41:
URL: https://github.com/apache/maven-shared-utils/pull/41
@michael-o
----------------------------------------------------------------
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] [maven-shared-utils] michael-o commented on a change in pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #41:
URL: https://github.com/apache/maven-shared-utils/pull/41#discussion_r432415800
##########
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:
I don't agree with this because have not opened this stream and it is not our task to close it. The one who opens the stream must close 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] michael-o commented on a change in pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #41:
URL: https://github.com/apache/maven-shared-utils/pull/41#discussion_r432427040
##########
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:
OK, leave as is and file a bug about the incorrect behavior. This should then be fixed in a subsequent release. Allso add documenation here, that behavior is wrong.
##########
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:
OK, leave as is and file a bug about the incorrect behavior. This should then be fixed in a subsequent release. Allso add documenation here that behavior is wrong.
----------------------------------------------------------------
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] [maven-shared-utils] elharo commented on a change in pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #41:
URL: https://github.com/apache/maven-shared-utils/pull/41#discussion_r432438490
##########
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:
done: https://issues.apache.org/jira/browse/MSHARED-895
----------------------------------------------------------------
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] [maven-shared-utils] roxspring commented on pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs
Posted by GitBox <gi...@apache.org>.
roxspring commented on pull request #41:
URL: https://github.com/apache/maven-shared-utils/pull/41#issuecomment-661703131
> Question: Doesn't
>
> ```
> try ( InputStream in = is )
> ```
>
> end with closing the InputStream in (aka is)?
This has already been discussed here:
https://github.com/apache/maven-shared-utils/pull/41/files#r432415800
In short: Yes it closes the `InputStream` but the original code did too and changing the behaviour would be bad.
----------------------------------------------------------------
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] [maven-shared-utils] elharo commented on a change in pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs
Posted by 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
[GitHub] [maven-shared-utils] elharo merged pull request #41: [MSHARED-860] deprecate PropertyUtils constructor and clean up docs
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #41:
URL: https://github.com/apache/maven-shared-utils/pull/41
----------------------------------------------------------------
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