search

FasterXML / jackson-dataformats-binary

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

Respect `WRITE_ENUMS_USING_TO_STRING` in `EnumAsIonSymbolSerializer` #241

Closed jhhladky closed 3 years ago

jhhladky commented 3 years ago

We don't need to make any changes for the corresponding READ_ENUMS_USING_TO_STRING because the enum deserializer is the Jackson one which already checks for the feature.

mcliedtke commented 3 years ago

LGTM, will look to @cowtowncoder for if we can add this to the 2.12 branch/release.

cowtowncoder commented 3 years ago

@mcliedtke As long as you are happy with this & changing in a patch, I'm fine with it.

@jhhladky Happy to merge, thanks -- just one thing first: unless I have asked for and gotten CLA, from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

I'd need that before merging this. It's a one-time thing so if you have sent one just let me know and I'll double-check; and in future it's good for other contributions. Anyway if needed, it's usually easiest to print, fill & sign, scan/photo, email to info at fasterxml dot com.

Looking forward to merging this!

cowtowncoder commented 3 years ago

Addendum to CCLA received, so I'll proceed with this. Timing is good as I am thinking of release 2.12.2 soon now (for couple of important fixes) -- and there'll probably be longer wait for 2.12.3.

cowtowncoder commented 3 years ago

One other note: since is implied that Enum is to be serialized as symbol this might not be relevant, but outside of Ion, Enums may also be serialized using index (specified by another SerializationFeature or @JsonFormat(shape=Shape.NUMBER_INT) override). In case that was relevant.

jhhladky commented 3 years ago

Addendum to CCLA received, so I'll proceed with this. Timing is good as I am thinking of release 2.12.2 soon now (for couple of important fixes) -- and there'll probably be longer wait for 2.12.3.

Great to hear thanks! Do you have a date in mind for that release? Context is we have another change we'd like to try to squeeze in if possible (Should have the PR out today or Monday), but that will be mostly informed by your timeline.

cowtowncoder commented 3 years ago

@jhhladky Ok if you have something else coming, I can wait -- was thinking this weekend but that is not driven by any external reasons. So I can definitely wait until, for example, following weekend. So for now I will plan at earliest to release on friday next week (19th), unless I hear anything else. Feel free to reach out on your plans, I'd rather not features miss release just due to unlucky timing (or similarly rush others).