search

FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
310 stars 133 forks source link

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

Open jwijgerd opened 4 years ago

jwijgerd commented 4 years ago

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

cowtowncoder commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

cowtowncoder commented 4 years ago

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 commented 4 years ago

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

cowtowncoder commented 4 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

cowtowncoder commented 3 years ago

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

cowtowncoder commented 3 years ago

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