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

[Avro] Ease future customization by unifying place where a new visitor is created. #292

Closed MichalFoksa closed 3 years ago

MichalFoksa commented 3 years ago

To ease customization by a user I suggest to add protected method AvroFormatVisitorWrapper (ex VisitorFormatWrapperImpl) createVisitorWrapper() into AvroFormatVisitorWrapperwhich creates and returns a new instance of visitor wrapper. This method would be used on each place where new visitor instance is created in ArrayVisitor, MapVisitor and RecordVisitor.

By having this in place it is easier to add / modify existing functionality by a user. He just needs to extend existing visitor wrapper and override particular method (just ask me how I know it).

```
protected AvroFormatVisitorWrapper createVisitorFormatWrapperImpl() {
    return new AvroFormatVisitorWrapper(_schemas, _provider);
}
```
cowtowncoder commented 3 years ago

While I am +1 for improving naming, isn't this backwards-incompatible change, and something that can't really go in 2.13 but should wait for 3.0?

MichalFoksa commented 3 years ago

:( OK - rename is undone

cowtowncoder commented 3 years ago

Ok so I think I can see why this is needed. I added a note where naming seems confusing wrt method -- I think I can see why naming of the class is unfortunate.

Once we get past that, maybe it would make sense to also file a follow-up issue about actual renaming like you suggested first. I'd have to think if it actually could go in 2.14, after all, or at least in 3.0.

cowtowncoder commented 3 years ago

LGTM! Will merge.

MichalFoksa commented 3 years ago

Thanks!