You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "shashankrnr32 (via GitHub)" <gi...@apache.org> on 2023/07/24 18:01:15 UTC
[GitHub] [commons-lang] shashankrnr32 opened a new pull request, #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
shashankrnr32 opened a new pull request, #1086:
URL: https://github.com/apache/commons-lang/pull/1086
Noticed this change recently which removed the usage of the method `getValue` to get the field value. The custom implementations of this class would never call the `getValue` method with custom implementations
@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
[GitHub] [commons-lang] codecov-commenter commented on pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1086:
URL: https://github.com/apache/commons-lang/pull/1086#issuecomment-1679521577
## [Codecov](https://app.codecov.io/gh/apache/commons-lang/pull/1086?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1086](https://app.codecov.io/gh/apache/commons-lang/pull/1086?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (455e1b1) into [master](https://app.codecov.io/gh/apache/commons-lang/commit/60b55f21b4b5e9fb5a655c588f2a212a88ca9483?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (60b55f2) will **increase** coverage by `0.13%`.
> Report is 113 commits behind head on master.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1086 +/- ##
============================================
+ Coverage 92.08% 92.22% +0.13%
- Complexity 7501 7552 +51
============================================
Files 195 197 +2
Lines 15720 15801 +81
Branches 2897 2923 +26
============================================
+ Hits 14476 14572 +96
+ Misses 670 659 -11
+ Partials 574 570 -4
```
| [Files Changed](https://app.codecov.io/gh/apache/commons-lang/pull/1086?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...mmons/lang3/builder/ReflectionToStringBuilder.java](https://app.codecov.io/gh/apache/commons-lang/pull/1086?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvYnVpbGRlci9SZWZsZWN0aW9uVG9TdHJpbmdCdWlsZGVyLmphdmE=) | `92.92% <100.00%> (+1.01%)` | :arrow_up: |
... and [13 files with indirect coverage changes](https://app.codecov.io/gh/apache/commons-lang/pull/1086/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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
[GitHub] [commons-lang] garydgregory closed pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory closed pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
URL: https://github.com/apache/commons-lang/pull/1086
--
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
[GitHub] [commons-lang] garydgregory commented on a diff in pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1086:
URL: https://github.com/apache/commons-lang/pull/1086#discussion_r1306651429
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionToStringBuilder.java:
##########
@@ -703,13 +703,11 @@ public Class<?> getUpToClass() {
*
* @throws IllegalArgumentException
* see {@link java.lang.reflect.Field#get(Object)}
- * @throws IllegalAccessException
- * see {@link java.lang.reflect.Field#get(Object)}
*
* @see java.lang.reflect.Field#get(Object)
*/
- protected Object getValue(final Field field) throws IllegalAccessException {
Review Comment:
Hello @shashankrnr32
Thank you for your update.
This PR attempts to restore compatibility with the `getValue()` method but changes that method's definition and breaks compatibility differently. For example, if I have a subclass that overrides `getValue()` using the released signature, that subclass will no longer compile if this patch is applied. The test in this PR does not reflect that a subclass can throw `IllegalAccessException` in the `getValue()` method signature, so the test is not checking for full compatibility. See git master for a fix.
--
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
[GitHub] [commons-lang] shashankrnr32 commented on a diff in pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "shashankrnr32 (via GitHub)" <gi...@apache.org>.
shashankrnr32 commented on code in PR #1086:
URL: https://github.com/apache/commons-lang/pull/1086#discussion_r1294890673
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionToStringBuilder.java:
##########
@@ -703,13 +703,11 @@ public Class<?> getUpToClass() {
*
* @throws IllegalArgumentException
* see {@link java.lang.reflect.Field#get(Object)}
- * @throws IllegalAccessException
- * see {@link java.lang.reflect.Field#get(Object)}
*
* @see java.lang.reflect.Field#get(Object)
*/
- protected Object getValue(final Field field) throws IllegalAccessException {
Review Comment:
As such, the method `Reflection.getUnchecked` does not throw any checked exception. By keeping this, i would either have to rethrow that in `appendFieldsIn` method or throw some `RuntimeException` so that i dont have to add checked exceptions to all the methods.
So, I removed it here. Open to suggestions
1. Catch IllegalAccessException in the method `appendFieldsIn` and throw some RuntimeException
2. Or rethrow the same exception. (Dont think this should be opted anyways)
--
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
[GitHub] [commons-lang] shashankrnr32 commented on pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "shashankrnr32 (via GitHub)" <gi...@apache.org>.
shashankrnr32 commented on PR #1086:
URL: https://github.com/apache/commons-lang/pull/1086#issuecomment-1679312608
> Hi @shashankrnr32 Thank you for your PR! This PR missing a unit test to avoid a future regression.
@garydgregory Added `ReflectionToStringBuilderCustomImplementationTest` that overrides the `getValue` and modifies the generated field value by the base class. If this behaviour changes in the future, the test will break.
--
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
[GitHub] [commons-lang] garydgregory commented on a diff in pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1086:
URL: https://github.com/apache/commons-lang/pull/1086#discussion_r1293399612
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionToStringBuilder.java:
##########
@@ -703,13 +703,11 @@ public Class<?> getUpToClass() {
*
* @throws IllegalArgumentException
* see {@link java.lang.reflect.Field#get(Object)}
- * @throws IllegalAccessException
- * see {@link java.lang.reflect.Field#get(Object)}
*
* @see java.lang.reflect.Field#get(Object)
*/
- protected Object getValue(final Field field) throws IllegalAccessException {
Review Comment:
Why are you removing `IllegalAccessException`?
--
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
[GitHub] [commons-lang] garydgregory commented on pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1086:
URL: https://github.com/apache/commons-lang/pull/1086#issuecomment-1694710578
Closing, see the comment.
--
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
[GitHub] [commons-lang] garydgregory commented on pull request #1086: Honor `getValue` method of ReflectionToStringBuilder & its subclasses to get the field value
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1086:
URL: https://github.com/apache/commons-lang/pull/1086#issuecomment-1694514774
https://issues.apache.org/jira/projects/LANG/issues/LANG-1710
--
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