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

Jump to bottom

[TEXT-190] Document negative limit for WordUtils abbreviate method #181

Closed
wants to merge 3 commits into from
Closed

[TEXT-190] Document negative limit for WordUtils abbreviate method #181

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c3e2b3
Update WordUtils.java
bjmbing Oct 29, 2020
ab55650
Add @javadoc for WordUtils.abbreviate lower
bjmbing Oct 29, 2020
50431e3
update javadoc
bjmbing Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/apache/commons/text/WordUtils.java
View file
Open in desktop
Expand Up @@ -859,7 +859,7 @@ public static boolean isDelimiter(final int codePoint, final char[] delimiters)
public static String abbreviate(final String str, int lower, int upper, final String appendToEnd) {
Validate.isTrue(upper >= -1, "upper value cannot be less than -1");
Validate.isTrue(upper >= lower || upper == -1, "upper value is less than lower value");

Validate.isTrue(lower >= 0, "lower value cannot be lower than 0");
Copy link
Member

Choose a reason for hiding this comment

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

@bjmbing thanks for the pull request. Initially this change made sense to me. Then looking at the abbreviate code, looks like the lower limit is used for StringUtils.indexOf(str, " ", /* startPos */ lower).

StringUtils.indexOf javadocs say: "* @param startPos the start position, negative treated as zero".

Currently, if you pass -1 or any other negative value, it is treated as zero, in accord with the StringUtils.indexOf javadocs.

I think a better solution would be to document that negative values are treated as zero, essentially replicating the StringUtils docs. This way, existing code using negative values won't get a runtime exception (which Validate.isTrue would do). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think the solution you provided is good.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the prompt reply @bjmbing! If you update this PR with something similar to the javadocs from StringUtils for the lower limit, we can then create a JIRA issue, and merge this PR :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kinow . I have updated the javadoc for lower in WordUtils.abbreviate.

if (StringUtils.isEmpty(str)) {
return str;
}
Expand Down