-
-
Notifications
You must be signed in to change notification settings - Fork 149
[Avro] Ease future customization by unifying place where a new visitor is created. #292
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
[Avro] Ease future customization by unifying place where a new visitor is created. #292
Conversation
| import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonArrayFormatVisitor; | ||
| import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonFormatTypes; | ||
| import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonFormatVisitable; | ||
| import org.apache.avro.Schema; |
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.
Please do not reorder import statements automatically in future... I use ordering from JDK types to non-Jackson types to Jackson types on purpose.
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.
Right - reordering is undone.
|
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? |
|
:( OK - rename is undone |
| _provider = p; | ||
| } | ||
|
|
||
| protected VisitorFormatWrapperImpl createVisitorWrapper() { |
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.
This seems odd from naming perspective: type with method to create what seems like (but isn't) a copy.
Would it conceptually be more somethnig like "createChildWrapper()" or "createChildInstance()" or something?
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.
I have chosen createChildWrapper. Did not find better name.
|
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. |
|
LGTM! Will merge. |
|
Thanks! |
To ease customization by a user I suggest to add protected method
AvroFormatVisitorWrapper(exVisitorFormatWrapperImpl) createVisitorWrapper() intoAvroFormatVisitorWrapperwhichcreates 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.