-
Notifications
You must be signed in to change notification settings - Fork 31
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 record support #163
Add record support #163
Conversation
@cowtowncoder Sorry for being quiet some time, I had a lot of things on my head. This is the corrected version of previous PR, sorry for the delay! |
jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/BeanPropertyReader.java
Show resolved
Hide resolved
jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/RecordsHelpers.java
Show resolved
Hide resolved
jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/RecordsHelpers.java
Outdated
Show resolved
Hide resolved
@TomaszGaweda Thank you very much for follow up here! This looks very good, but I want to take time to properly review this later today. Just wanted to give quick notes first. I really like the idea of getting full Record support for jackson-jr 2.18! |
Thank you for your patience. I know I f..ked up last time, sorry for that... Hopefully this time it's actually a good PR. Take your time, I definetely don't want to rush it or shout "faster, faster!" at you ;) |
@TomaszGaweda no problem at all -- I failed to detect the obvious case. We should be all good now. |
Thanks for the PR @TomaszGaweda, this was the missing item i required, 2.18 looks like I can finally swap out Jackson for Jackson-jr (for my hobby projects) 🥳🥳, and maybe then even in prod. |
jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/BeanPropertyIntrospector.java
Outdated
Show resolved
Hide resolved
// too, regardless of `Feature.USE_FIELD_MATCHING_GETTERS` | ||
final boolean isFieldNameGettersEnabled = JSON.Feature.USE_FIELD_MATCHING_GETTERS.isEnabled(features) | ||
|| RecordsHelpers.isRecordType(currType); | ||
final boolean isFieldNameGettersEnabled = JSON.Feature.USE_FIELD_MATCHING_GETTERS.isEnabled(features); |
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.
Probably accidental change; will revert.
jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/BeanReader.java
Outdated
Show resolved
Hide resolved
Getting there @TomaszGaweda . One thing I noticed is that handling of 1-parameter constructor is before Record constructors; should probably change order and, more importantly, verify that such Records (with a single I also made some changes I did to the earlier PR. EDIT: added tests and changed handling to work as expected. |
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.
Ok made couple of changes, I think it's ready to merge.
Thanks a lot @cowtowncoder for your support and fixes! I was off on the weekend, so unfortunatelly didn't see comments before you've addressed them yourself ;p |
@TomaszGaweda did not expect you to -- but I happened to have time :) Thank you once again for contributing this: it's a big formerly missing piece that will be pretty valuable. |
This PR adds Java 17's record support.
This is an improved version of #148
It supports different property order, nesting and nulls.
Closes #162