Honor getValue method of ReflectionToStringBuilder & its subclasses to get the field value
#1086
Conversation
There was a problem hiding this comment.
Hi @shashankrnr32
Thank you for your PR!
This PR missing a unit test to avoid a future regression.
| * | ||
| * @see java.lang.reflect.Field#get(Object) | ||
| */ | ||
| protected Object getValue(final Field field) throws IllegalAccessException { |
There was a problem hiding this comment.
Why are you removing IllegalAccessException?
There was a problem hiding this 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
- Catch IllegalAccessException in the method
appendFieldsInand throw some RuntimeException - Or rethrow the same exception. (Dont think this should be opted anyways)
There was a problem hiding this 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.
@garydgregory Added |
Codecov Report
@@ 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
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Closing, see the comment. |
Noticed this change recently which removed the usage of the method
getValueto get the field value. Custom implementations of thegetValuewould not be used anymore.@garydgregory