You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/10/29 20:12:18 UTC
[GitHub] [commons-text] bjmbing opened a new pull request #181: Add validate for WordUtils.java
bjmbing opened a new pull request #181:
URL: https://github.com/apache/commons-text/pull/181
lower < 0 has no defined behavior in the document. Although it works similar to lower = 0. There is no related document for 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] [commons-text] kinow commented on a change in pull request #181: Add validate for WordUtils.java
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #181:
URL: https://github.com/apache/commons-text/pull/181#discussion_r514563716
##########
File path: src/main/java/org/apache/commons/text/WordUtils.java
##########
@@ -828,7 +828,7 @@ public static boolean isDelimiter(final int codePoint, final char[] delimiters)
*
* @param str the string to be abbreviated. If null is passed, null is returned.
* If the empty String is passed, the empty string is returned.
- * @param lower the lower limit.
+ * @param lower the lower limit; < -1 value is treated as 0.
Review comment:
Might be better ", negative treated as zero", as any negative value, not only `-1` is treated as 0
----------------------------------------------------------------
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] [commons-text] kinow commented on pull request #181: [TEXT-190] Document negative limit for WordUtils abbreviate method
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #181:
URL: https://github.com/apache/commons-text/pull/181#issuecomment-719020194
I've created https://issues.apache.org/jira/browse/TEXT-190 for this issue, updated the title. Once the PR passes review, we should be ready to merge and include in the changelog :+1: Thanks @bjmbing !
----------------------------------------------------------------
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] [commons-text] bjmbing commented on a change in pull request #181: Add validate for WordUtils.java
Posted by GitBox <gi...@apache.org>.
bjmbing commented on a change in pull request #181:
URL: https://github.com/apache/commons-text/pull/181#discussion_r514554074
##########
File path: src/main/java/org/apache/commons/text/WordUtils.java
##########
@@ -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");
Review comment:
Hi @kinow . I have updated the javadoc for lower in WordUtils.abbreviate.
----------------------------------------------------------------
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] [commons-text] bjmbing commented on a change in pull request #181: Add validate for WordUtils.java
Posted by GitBox <gi...@apache.org>.
bjmbing commented on a change in pull request #181:
URL: https://github.com/apache/commons-text/pull/181#discussion_r514547045
##########
File path: src/main/java/org/apache/commons/text/WordUtils.java
##########
@@ -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");
Review comment:
Yeah I think the solution you provided is good.
----------------------------------------------------------------
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] [commons-text] kinow closed pull request #181: [TEXT-190] Document negative limit for WordUtils abbreviate method
Posted by GitBox <gi...@apache.org>.
kinow closed pull request #181:
URL: https://github.com/apache/commons-text/pull/181
----------------------------------------------------------------
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] [commons-text] bjmbing commented on pull request #181: [TEXT-190] Document negative limit for WordUtils abbreviate method
Posted by GitBox <gi...@apache.org>.
bjmbing commented on pull request #181:
URL: https://github.com/apache/commons-text/pull/181#issuecomment-719023040
I changed the doc as suggested. @kinow . Thank you for your help
----------------------------------------------------------------
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] [commons-text] kinow commented on a change in pull request #181: Add validate for WordUtils.java
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #181:
URL: https://github.com/apache/commons-text/pull/181#discussion_r514544829
##########
File path: src/main/java/org/apache/commons/text/WordUtils.java
##########
@@ -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");
Review comment:
@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?
----------------------------------------------------------------
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] [commons-text] kinow commented on pull request #181: [TEXT-190] Document negative limit for WordUtils abbreviate method
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #181:
URL: https://github.com/apache/commons-text/pull/181#issuecomment-719024328
JIRA issue resolved, added your change with credit/thanks to @bjmbing, merged! Thanks again @bjmbing !
----------------------------------------------------------------
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] [commons-text] kinow commented on a change in pull request #181: Add validate for WordUtils.java
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #181:
URL: https://github.com/apache/commons-text/pull/181#discussion_r514548455
##########
File path: src/main/java/org/apache/commons/text/WordUtils.java
##########
@@ -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");
Review comment:
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 :-)
----------------------------------------------------------------
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