Add generic Value#convert(Object) method #7076
Conversation
| * | ||
| * @param object the object to convert | ||
| * @return the equivalent {@link Value} | ||
| * @throws IllegalArgumentException if not able to convert the object to {@link Value} |
There was a problem hiding this comment.
Might want to throw a checked exception to more forcibly signal to callers that they need to handle this.
There was a problem hiding this comment.
Decided against this:
- This method is analagous to jackson ObjectMapper#convertValue, which only throws a runtime exception. This API is quite popular and I've used it myself without issue for years, suggesting that a runtime exception is sufficient.
- I'm not sure what checked exception would be most appropriate to throw. Probably some subclass of
IOException, and we'd probably want to introduce our own dedicated exception. In contrast,IllegalArgumentExceptionpretty perfectly describes the error and seems appropriate to throw.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7076 +/- ##
============================================
+ Coverage 90.06% 90.11% +0.04%
- Complexity 7320 7338 +18
============================================
Files 825 825
Lines 22048 22084 +36
Branches 2179 2188 +9
============================================
+ Hits 19857 19900 +43
+ Misses 1511 1504 -7
Partials 680 680 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jkwatson
left a comment
jkwatson
left a comment
There was a problem hiding this comment.
seems ok to me. Needs a rebase to get the apidiffs up to date, when you get a moment.
|
I'd suggest holding on this for a bit longer while we figure out the impact of extending attributes to support any value, just in case. |
should we move this to a Draft, then? |
|
Marking "Ready for review" now that #7814 is merged. |
| * @throws IllegalArgumentException if not able to convert the object to {@link Value} | ||
| */ | ||
| @SuppressWarnings("ThrowsUncheckedException") | ||
| static Value<?> convert(Object object) throws IllegalArgumentException { |
There was a problem hiding this comment.
do we think we would use the Object-ness? or would we just use convert(Map) and convert(List)?
e.g. in the structured log attribute mapping case, would we do instanceof and use regular attributes where possible, and only use Value attributes when it's a Map or List? or would we just convert(Object) all of them?
There was a problem hiding this comment.
This is a good point!
Given #7814 and the perf penalty associated with using Value to needless represent primitive / array of primitive attributes, I think we could steer people away from doing this by only publicly exposing Map and List variants. I.e.
public static Value<?> convert(Map<String, Object>) {}
public static Value<?> convert(List<Object>) {}
private static Value<?> convert(Object) {}
4 participants
No description provided.