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.

Sign up for GitHub

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

[LANG-1448] StringUtils Javadoc wrong for containsOnly and containsNone #1308

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[LANG-1448] StringUtils Javadoc wrong for containsOnly and containsNone #1308

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions src/main/java/org/apache/commons/lang3/StringUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,13 +1191,13 @@ public static boolean containsIgnoreCase(final CharSequence str, final CharSeque
* An empty CharSequence (length()=0) always returns true.</p>
*
* <pre>
* StringUtils.containsNone(null, *) = true
* StringUtils.containsNone(*, null) = true
* StringUtils.containsNone("", *) = true
* StringUtils.containsNone("ab", '') = true
* StringUtils.containsNone("abab", 'xyz') = true
* StringUtils.containsNone("ab1", 'xyz') = true
* StringUtils.containsNone("abz", 'xyz') = false
* StringUtils.containsNone(null, *) = true
* StringUtils.containsNone("", *) = true
* StringUtils.containsNone(*, null) = true
* StringUtils.containsNone(*, []) = true
* StringUtils.containsNone("abc", ['x', 'y']) = true
* StringUtils.containsNone("abc", ['1', '.']) = true
Copy link
Member

Choose a reason for hiding this comment

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

Hi @ParanoidUser

Using ['1', '.'] is more confusing IMO instead of 'x', 'y', 'z'. You use * and [text] which won't compile either, which ironically, is what the PR's description complains about!

Are we to assume that '.' is a regular expression or a the actual character dot in ['1', '.'] since it's in between y and z examples?

The reason there are many examples in Javadoc is to avoid using BNF or RegEx. I can see an exception for using * but anything more clever than that is going to be counter productive IMO.

Copy link
Contributor Author

@ParanoidUser ParanoidUser Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @garydgregory, thank you for your feedback. While reviewing the StringUtils class, I noticed that non-compilable Javadoc examples are quite common. While we could make these examples compilable, I believe their primary purpose is to demonstrate method behavior clearly and concisely.

Let's look at the StringUtils#containsNone(CharSequence, char...) Javadoc as an example:

StringUtils.containsNone(null, *)           = true  // unexpected token *
StringUtils.containsNone("", *)             = true  // unexpected token *
StringUtils.containsNone(*, null)           = true  // unexpected token * + ambiguous method call with null
StringUtils.containsNone(*, [])             = true  // unexpected tokens * and [] 
StringUtils.containsNone("abc", ['x', 'y']) = true  // array type expected
StringUtils.containsNone("abc", ['y', 'z']) = true  // array type expected
StringUtils.containsNone("abc", ['b', 'z']) = false // array type expected

Here is compilable version:

StringUtils.containsNone(null, "abc");        = true
StringUtils.containsNone("", "abc");          = true
StringUtils.containsNone("", (char[]) null);  = true // not very obvious what this sample trying to show 
StringUtils.containsNone("abc", new char[0]); = true // or StringUtils.containsNone("abc")
StringUtils.containsNone("abc", 'x', 'y');    = true
StringUtils.containsNone("abc", 'y', 'z');    = true
StringUtils.containsNone("abc", 'b', 'z');    = false

Or we can continue using special symbols like * (for any value) and [] (for empty arrays/varargs) in documentation, as they better communicate the method's behavior:

StringUtils.containsNone(null, *)         = true  // second argument doesn't matter, due to null
StringUtils.containsNone("", *)           = true  // second argument doesn't matter, due to empty-string
StringUtils.containsNone(*, null)         = true  // first argument doesn't matter, due to null
StringUtils.containsNone(*, [])           = true  // first argument doesn't matter, due to empty-array
StringUtils.containsNone("abc", 'x', 'y') = true  // valid varargs
StringUtils.containsNone("abc", 'y', 'z') = true  // valid varargs 
StringUtils.containsNone("abc", 'b', 'z') = false // valid varargs

What are your thoughts on which style we should use for the documentation?

Copy link
Member

Choose a reason for hiding this comment

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

Hello @ParanoidUser

If I had to pick only one style, I would pick "compilable". In an ideal world, I could imagine Javadoc like:

Does this and that.
<p>
For example:
</p>
<pre>{@code
StringUtils.bla("aaa", "bb") == true;
StringUtils.bla("aaa", "zz") == false;
}
<p>
In general, where `*` means any String:
</p>
<pre>{@code
StringUtils.bla("aaa", *) == true;
StringUtils.bla(*, "zz") == false;
}

WDYT?

* StringUtils.containsNone("abc", ['b', 'z']) = false
* </pre>
*
* @param cs the CharSequence to check, may be null
Expand Down Expand Up @@ -1244,12 +1244,12 @@ public static boolean containsNone(final CharSequence cs, final char... searchCh
*
* <pre>
* StringUtils.containsNone(null, *) = true
* StringUtils.containsNone(*, null) = true
* StringUtils.containsNone("", *) = true
* StringUtils.containsNone("ab", "") = true
* StringUtils.containsNone("abab", "xyz") = true
* StringUtils.containsNone("ab1", "xyz") = true
* StringUtils.containsNone("abz", "xyz") = false
* StringUtils.containsNone(*, null) = true
* StringUtils.containsNone(*, "") = true
* StringUtils.containsNone("abc", "xy") = true
* StringUtils.containsNone("abc", "1.") = true
* StringUtils.containsNone("abc", "bz") = false
* </pre>
*
* @param cs the CharSequence to check, may be null
Expand All @@ -1273,13 +1273,13 @@ public static boolean containsNone(final CharSequence cs, final String invalidCh
* An empty CharSequence (length()=0) always returns {@code true}.</p>
*
* <pre>
* StringUtils.containsOnly(null, *) = false
* StringUtils.containsOnly(*, null) = false
* StringUtils.containsOnly("", *) = true
* StringUtils.containsOnly("ab", '') = false
* StringUtils.containsOnly("abab", 'abc') = true
* StringUtils.containsOnly("ab1", 'abc') = false
* StringUtils.containsOnly("abz", 'abc') = false
* StringUtils.containsOnly(null, *) = false
* StringUtils.containsOnly("", *) = true
* StringUtils.containsOnly(*, null) = false
* StringUtils.containsOnly(*, []) = false
* StringUtils.containsOnly("abab", ['a', 'b', 'c']) = true
* StringUtils.containsOnly("ab1", ['a', 'b', 'c']) = false
* StringUtils.containsOnly("abz", ['a', 'b', 'c']) = false
* </pre>
*
* @param cs the String to check, may be null
Expand Down Expand Up @@ -1310,9 +1310,9 @@ public static boolean containsOnly(final CharSequence cs, final char... valid) {
*
* <pre>
* StringUtils.containsOnly(null, *) = false
* StringUtils.containsOnly(*, null) = false
* StringUtils.containsOnly("", *) = true
* StringUtils.containsOnly("ab", "") = false
* StringUtils.containsOnly(*, null) = false
* StringUtils.containsOnly(*, "") = false
* StringUtils.containsOnly("abab", "abc") = true
* StringUtils.containsOnly("ab1", "abc") = false
* StringUtils.containsOnly("abz", "abc") = false
Expand Down
Loading