Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSHARED-1172] Deprecate redundant isEmptyString method #123

Merged
merged 4 commits into from Jan 1, 2023

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Nov 25, 2022

deprecate a string utility method that doesn't belong in this package, wasn't correctly documented, isn't tested, and is duplicated in 72 other places

@slawekjaranowski

deprecate a string utility method that doesn't belong in this package and is duplicated in 72 other places

@slawekjaranowski
@elharo elharo changed the title Update Xpp3DomUtils.java Deprecated redundant isEmptyString method Nov 25, 2022
@elharo elharo changed the title Deprecated redundant isEmptyString method [MSHARED-1172] Deprecate redundant isEmptyString method Dec 3, 2022
@elharo elharo requested a review from michael-o December 3, 2022 19:53
@elharo
Copy link
Contributor Author

elharo commented Dec 8, 2022

PTAl @michael-o

@@ -148,15 +148,15 @@ private static boolean isMergeChildren( Xpp3Dom dominant )
}

/**
* @param str The string to be checked.
* @return <code>true</code> in case string is empty <code>false</code> otherwise.
* @deprecated use <code>str == null || String.isBlank(str)</code> (Java 11+)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not recommending https://github.com/apache/maven-shared-utils/blob/f4383c55f77ad4480f146481bf3192bc52c3af30/src/main/java/org/apache/maven/shared/utils/StringUtils.java#L145 as replacement? Or should we rather deprecate that method as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine as well

Copy link
Contributor Author

@elharo elharo Jan 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO

JDK > org,apache.commons > Guava > maven-shared-utils > plexus-utils > random classes where it doesn't belong

Realistically, this method is reinvented so often and so frequently, we'll never get down to a single version. However this case is particularly egregious.

@elharo elharo merged commit c002482 into master Jan 1, 2023
@elharo elharo deleted the elharo-patch-1 branch January 1, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants