-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix: 5078 Add new MonthDeserializer
#5122
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
Fix: 5078 Add new MonthDeserializer
#5122
Conversation
// TODO : Figure out how to make this pass. | ||
// try { | ||
// Month result = mapper.readerFor(Month.class).readValue("\"\""); | ||
// fail("Should not pass"); | ||
// } catch (MismatchedInputException e) { | ||
// verifyException(e, "Cannot coerce empty String"); | ||
// } |
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.
Current implementation returns null
with or without below CoercionSetting 🤔.
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.
This may be way to go as MonthDayDeser... also behaves the same
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 figured it out -- is bit complicated but rules changed when LogicalType
became DateTime
instead of Enum
(technically is an Enum but logically DateTime (more refined)).
So I changed test a bit.
@@ -17,9 +12,7 @@ public JavaTimeDeserializerModifier() { } | |||
@Override | |||
public ValueDeserializer<?> modifyEnumDeserializer(DeserializationConfig config, JavaType type, | |||
BeanDescription.Supplier beanDescRef, ValueDeserializer<?> defaultDeserializer) { | |||
if (type.hasRawClass(Month.class)) { |
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.
We can remove this class altogether (I'll do that)
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.
Thanksss I was actually 50/50 on this 😅 forgot to mention.
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.
LGTM!
Excellent, thank you @JooHyukKim ! |
this part of #5078