Skip to content
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

Added test case for nullable enum that defaults to null and defaults … #1

Merged
merged 1 commit into from
Dec 29, 2020

Conversation

darmstrong1
Copy link

…to an enum value is not null.

Hi Kossi,

Thank you for looking at this issue so quick!
Your solution looks good.

I added one test case. I wanted to see if I could make it work for a nullable enum that defaults to null and defaults to a value in the enum if it is not null.
I had to create a data class with just the enum in it and give it the default value, then make that enum class the nullable field for it to work. I could not think of another way to do it since I can't give an enum two default values. Even if I could, I don't know how it would choose which to use.
I just wanted to see if it were possible because I'm sure cases will come up where the value can be null and if it is not, people will want the value to have a default for backward compatibility.

@kossi
Copy link
Owner

kossi commented Dec 29, 2020

Ok thanks, I'll merge this to that PR.

Yeah sorry. I don't know if the structure should be with enum defaults a bit different. Example separate annotations for enum defaults or something like that. Or the schemata structure for field defaults and enum defaults are different but let's see

@kossi kossi merged commit 39070af into kossi:avro-enum-defaults Dec 29, 2020
@kossi
Copy link
Owner

kossi commented Dec 29, 2020

Sorry btw about the force-pushing stuff on my branch on your commit. Or removed one unused import that I had added earlier.

Reasoning for doing the force-push is that I thought commit history included the merge commit message. Well anyways it probably doesn't really matter or the PR just now two commits

@darmstrong1
Copy link
Author

kossi, no problem.
I was thinking the same thing as you, that it may be better to have a separate enum for avro defaults since they have a different purpose from "regular" defaults.
I see thake created an issue to support enum defaults, so he seems to have the same idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants