Skip to content
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

JSR310DateTimeDeserializerBase#withDateFormate implementations without this as base class #276

Closed
raman-babich opened this issue Jun 16, 2023 · 2 comments · Fixed by #278

Comments

@raman-babich
Copy link
Contributor

I was analyzing the sources and have found some strange things.

  • LocalTimeDeserializer
  • YearDeserializer
  • LocalDateTimeDeserializer
  • MonthDayDeserializer
  • OffsetTimeDeserializer
  • YearMonthDeserializer

classes don't use this as base when ovverriding JSR310DateTimeDeserializerBase#withDateFormate and therefore we can loose leniency and shape. What is the real intention of such code or is it a bug? If this is inteded behaviour, e.g. because implementations don't use leniency and shape and only care about formatter, there is another strange thing why does withShape is ignored by returning this almost everywhere but ignoring 'withLeniency' leads to new deser made?

LocalTimeDeserializer as an example

@cowtowncoder
Copy link
Member

Without remembering details here I would lean towards it being a simple bug (oversight): whenever creating something using withXxx() it does seem like this ought to be passed.

As to withShape() returning this, that is likely due to that particular deserializer not using shape for changing behavior. Shape is more commonly used with serializers. But it is worth checking for individual deserializers if you have time to do so.

Thank you for doing good due diligence here!

@raman-babich
Copy link
Contributor Author

Ok, I will make a pr after merge of #272 if I have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants