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

implement property and optionalProperty(String, JsonFormatVisitable, JavaType) methods #220

Open
wants to merge 1 commit into
base: 2.12
Choose a base branch
from

Conversation

jwijgerd
Copy link

implemented the propery methods that take property name and type as inputs. This is needed to support custom (de)serializers that are using these methods. This can only be used with the DefaultTagGenerator so an IllegalStateException will be thrown if this gets mixed with properties annotated with @JsonProperty.index

I created this one on the 2.12 branch because my master branch was not compiling. can be cherry picked to master as well

…nputs. This is needed to support custom (de)serializers that are using these methods. This can only be used with the DefaultTagGenerator so an IllegalStateException will be thrown if this gets mixed with properties annotated with @JsonProperty.index
@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 14, 2020

Thank you for contributing this! I'll have to take a look to make sure I understand it, but sounds reasonable. And 2.12 branch is actually very good choice; I can merge it up to master easily and 2.12.0 should be released within maybe a month.

I also think we already have your CLA which simplifies things.

@jwijgerd
Copy link
Author

Thanks for picking this up so soon. I recently contributed something to jackson-datatype-joda but can't remember signing a CLA so you may want to check that.. Happy to sign if needed!

@cowtowncoder
Copy link
Member

@jwijgerd I did check our repo, and there's CLA for jackson 2.5, seems like afterburner contribution (which would also mean no need for another for Joda). :)

@cowtowncoder
Copy link
Member

Quick question: any chance to provide a simple unit test with this? (ideally something failing without this, passing with fix)

The other thing was possible concern wrt adding a new method in interface TagGenerator, regarding backwards compatibility, but I guess that can't be helped (until baseline is increased to Java 8 allowing default impl).

@jwijgerd
Copy link
Author

Sure thing, I will provide a test. Let me think about TagGenerator interface a bit and get back to you

@cowtowncoder
Copy link
Member

Sounds good! I'll make sure this can get in 2.12.0-rc1 (planning to release over next weekend, in 7 days, if all goes well).

@jwijgerd
Copy link
Author

@cowtowncoder Sorry I wasn't able to work on this last week, hope to get the test in today or tomorrow

@cowtowncoder
Copy link
Member

@jwijgerd np, thank you for update! I am hoping to release 2.12.0-rc1 next weekend, if possible, would be great to include this -- but it is not the hard deadline, can include after, before 2.12.0 final.

@cowtowncoder
Copy link
Member

@jwijgerd At this point 2.12.0-rc2 is not yet out, but will be within a week -- so would be great to see if this could still get in?

@jwijgerd
Copy link
Author

@cowtowncoder will try to do my best, very hard to find some time atm!

@cowtowncoder
Copy link
Member

@jwijgerd Understood, we are all volunteers, so no pressure.

@cowtowncoder
Copy link
Member

Just noting that this could be targeted for 2.13 (no hurry)

@cowtowncoder cowtowncoder removed the 2.13 label Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants