-
Notifications
You must be signed in to change notification settings - Fork 132
Add converters for simple type projections #65
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
Add converters for simple type projections #65
Conversation
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.
That's a great start. We haven't discussed yet which types to convert. We should include LocalDate
, LocalDateTime
, LocalTime
, OffsetDateTime
and ZonedDateTime
. I'm not sure about Date
yet, InetAddr
is not supported by R2DBC.
|
||
@Override | ||
public Boolean convert(Row row) { | ||
return (Boolean) row.get(0); |
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.
Use row.get(0, Boolean.class)
to avoid downcasting.
This also applies for all other calls (except for theRowToNumberConverterFactory
).
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.
awesome. I will do this refactorings and create a test for the converters.
c1b350b
to
61f2966
Compare
hellp @mp911de PR is updated. |
Thanks a lot. The code looks good. Care to add a bit of Javadoc? Have a look into the other store modules that provide similar converters for inspiration. |
61f2966
to
5bdaafe
Compare
done. Let me know if there is anything else that I should do here. |
Looks good. We now should wait until #62 is merged to avoid further merge conflicts. |
Rename RowDataConverter to R2dbcConverters. Introduce R2dbcSimpleTypeHolder. Apply custom conversions check in MappingR2dbcConverter. Extend tests. Original pull request: #65.
That's merged and polished now. Thanks a lot! |
partial solution for #41