Support arbitrary-sized unions with N-ary sum type #168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fieldstrength
commented
Oct 15, 2021
| @@ -464,22 +466,17 @@ mkFieldTypeName namespaceBehavior = \case | |||
| S.Enum n _ _ _ -> [t| $(conT $ mkDataTypeName namespaceBehavior n)|] | |||
| where go = mkFieldTypeName namespaceBehavior | |||
| union = \case | |||
| [] -> | |||
There was a problem hiding this comment.
I'm not sure if anyone would take issue with this, but I'm personally very pleased to be ride of this special case. I was recently involved in designing a new schema and found it was inconvenient that Avro generation broke because of types whose values had simply not been defined yet, since Void ~ NS [] is a perfectly valid type, and it doesn't hurt anything to have such a type around while you're developing a schema, whereas fake or provisional values can be troublesome.
|
This looks good, thanks! Pme question: what would be the Haskell type reported to And could you please add some Readme section or an example/test about how to use / pattern match on N-sums? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi there, I've been interested to have this feature for some time, since in my company we've frequently needed to add to the
EitherNmodule whenever we add something. So I've taken a stab at implementing it. I used a sum type similar toNSfromgenerics-sop(but specialized to not be kind-polymorphic).Since this is a larger change I'm of course happy to hear the perspective of the maintainers and users and try to accommodate your preferences.
With the existing round-trip tests Im fairly confident the implementation is correct. I also ran another test to check the consistency with the existing implementation, which adds to my confidence. (That test is not included in this patch, because it removes the old types).
I'm not sure how well covered the unions are by the benchmark suite. I think the performance should at least not be too much different, but Im also interested to validate that and make adjustments if necessary. However, I wanted to start the conversation here first at least, to make sure there is buy-in for this change before going further.