[
https://issues.apache.org/jira/browse/AVRO-1767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15047504#comment-15047504
]
Ryan Blue commented on AVRO-1767:
---------------------------------
This looks good to me.
Just one note on having several changes in one patch though... I think it makes
sense most of the time to keep changes in separate commits/patches. I know that
increases overhead a bit, but then we have more ability to revert individual
changes that end up being problematic. For example, if we discover a bug in
this patch and want to back out the union handling change, we have to go back
to editing the code to sort out what is broken and what isn't. It also means
it's easier to review things (and faster) because the reviewer doesn't have to
find what differences are associated with each high-level change. Small,
independent patches are the easiest to work with in a lot of ways.
> Simplify union wrapping logic.
> ------------------------------
>
> Key: AVRO-1767
> URL: https://issues.apache.org/jira/browse/AVRO-1767
> Project: Avro
> Issue Type: Improvement
> Components: javascript
> Reporter: Matthieu Monsch
> Assignee: Ryan Blue
> Attachments: AVRO-1767.patch
>
>
> Previously, union wrapping during `type.clone` calls would be done in place
> of normal copy. Now it is done as a backup if a normal copy fails. This is
> more consistent with the documentation, and makes it much simpler to wrap
> nested unions.
> See here for more information and examples:
> https://github.com/mtth/avsc/issues/16
> Other changes:
> + Add an optional argument to `type.getName` to expose built-in type names
> (useful to implement an "optional" logical type for example).
> + Document (and rename) `assertLogicalTypes` parsing option.
> + Tweak logical type validity check, enabling error hooks to work even when a
> logical type is valid for a subset of its underlying type's values (e.g.
> when
> defining an "even integer" type). Previously the path would have been
> correct
> but the type would have been that of the underlying one.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
