-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Support alternate radixes for numeric values #5445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.x
Are you sure you want to change the base?
Conversation
| import tools.jackson.databind.util.ClassUtil; | ||
| import tools.jackson.databind.util.Converter; | ||
|
|
||
| import static com.fasterxml.jackson.annotation.JsonFormat.DEFAULT_RADIX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| } | ||
|
|
||
| JsonFormat.Value formatValue = EMPTY_FORMAT; | ||
| if (v0 != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could this be null when it is assigned on line 61?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| } | ||
|
|
||
| @Override | ||
| public JsonFormat.Value findPropertyFormat(MapperConfig<?> config, Class<?> baseType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am bit confused here -- why are changes needed? Shouldn't override handling work for radix just like all other properties? If not, why not (are fixes needed in jackson-annotations?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is to consider default format out of ConfigOverrides which has the lowest precedence. This way we can make the a given radix apply to all integral types. This is what you meant here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added a test for this behavior DifferentRadixNumberFormatTest#testIntSerializedAsHexStringWithDefaultRadix
| JsonFormat.Value v2 = null; | ||
| AnnotationIntrospector intr = config.getAnnotationIntrospector(); | ||
| if (intr != null) { | ||
| AnnotatedMember member = getMember(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we mixing in member annotations now -- this changes semantics of the method.
EDIT: never mind, that was done via findFormatOverrides() which does about same. But maybe call that method instead of inlining the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Do not remember why I inlined in #5317
Add support for radices using the added to
JsonFormatradix attribute in FasterXML/jackson-annotations#321. The initial work for the PR was done in issue #5317, and the original request was #221.