You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "arthurscchan (via GitHub)" <gi...@apache.org> on 2023/11/27 20:58:21 UTC
[PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
arthurscchan opened a new pull request, #1139:
URL: https://github.com/apache/commons-lang/pull/1139
The Javadoc description comments for most methods of the Conversion class, it contains a detailed description of the reason for some expected exceptions thrown by each method. These exceptions include but are not limited to different `IndexOutOfBoundsException` or `NullPointerException`. It is found that all variants of the `hexTo*()` method will throw `StringIndexOutOfBoundsException` if `srcPos + nHex > src.length` but this is not documented. Thus this PR provides an improvement of the relative Javadoc, mentioning that these methods could throw `StringIndexOutOfBoundsException`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-3248907182
After reworking the class locally to throw IEA instead of `[Array|String]IndexOutOfBoundsException`, it looks like we work too hard for the happy path when some of the APIs already document `[Array|String]IndexOutOfBoundsException`. Since this is a low-level API, it also makes more sense to keep closer to what the JRE does. We also have our Validate class which throws [Array|String]IndexOutOfBoundsException when validating array bounds. So I'll revert to the released style and update the Javadoc to be consistent.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "arthurscchan (via GitHub)" <gi...@apache.org>.
arthurscchan commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1845378990
I am ok with either way. But it may need to change other method also to remain consistency. For example, the other method [here](https://github.com/apache/commons-lang/blob/master/src/main/java/org/apache/commons/lang3/Conversion.java#L113), just throw IndexOutOfBoundsException directly and document that. @garydgregory
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-3245230006
Hi all, @arthurscchan , @elharo
I've reviewed this PR, the code, and pushed to git master with better input checks:
- Conversion.hexTo*() methods now throw `IllegalArgumentException` instead of `StringIndexOutOfBoundsException`
- The Javadoc is updated but lets the exception messages give the new details
- The exception messages are better
- The rest of the class will get similar updates
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1885203106
Letting code fail is fine and avoids extra work on the happy path. If an explicit check isn't needed, it isn't needed. The important thing is to document and test the desired behavior. As long as the behavior is tested, how the behavior is implemented is a detail.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1882292378
On further investigation, I'm not so sure. The API here is really strange. Since the indexes that can be out of bounds are passed in from client code, StringIndexOutOfBoundsException seems appropriate in at least some cases.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1885942495
It sounds like we are ok with this PR then. @elharo @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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-3245386836
Closing.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1845373304
@arthurscchan
I'm not so sure about this one. Maybe the original runtime exception should be rethrown as an `IllegalArgumentException`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1875288783
I agree with @garydgregory Throwing IndexOutOfBoundsExceptions from these methods i a bug. These should all be IllegalArgumentException. Filed https://issues.apache.org/jira/browse/LANG-1725
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory closed pull request #1139: LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class
URL: https://github.com/apache/commons-lang/pull/1139
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1843939386
## [Codecov](https://app.codecov.io/gh/apache/commons-lang/pull/1139?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`c52fec6`)](https://app.codecov.io/gh/apache/commons-lang/commit/c52fec6932b9f595e0a5ad22c18f561c228291a4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 92.14% compared to head [(`8043291`)](https://app.codecov.io/gh/apache/commons-lang/pull/1139?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 92.15%.
> Report is 8 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1139 +/- ##
============================================
+ Coverage 92.14% 92.15% +0.01%
- Complexity 7595 7596 +1
============================================
Files 200 200
Lines 15910 15910
Branches 2925 2925
============================================
+ Hits 14660 14662 +2
+ Misses 676 675 -1
+ Partials 574 573 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/commons-lang/pull/1139?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1875316925
Given that we do want to change this behavior, it's probably not a good idea to Javadoc the current behavior, so I suggest closing this PR. However, it is still very valuable for calling our attention to this issue. Thanks.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1875301469
> I agree with @garydgregory Throwing IndexOutOfBoundsExceptions from these methods i a bug. These should all be IllegalArgumentException. Filed https://issues.apache.org/jira/browse/LANG-1725
Agree as well.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1884987065
@elharo and all
Taking the same approach as `java.util.Arrays` seems reasonable but that class checks and throws exceptions (IAE and ArrayIndexOutOfBoundsException) explicitly instead of letting code fail. Thoughts?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LANG-1720: Fix Javadoc description of exceptions thrown for Conversion class [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1139:
URL: https://github.com/apache/commons-lang/pull/1139#issuecomment-1878607677
I agree this PR should be closed as is. I'll leave it open for now if @arthurscchan wants to update it with code changes instead.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org