Check type before using asInstanceOf
#706
Conversation
|
@pablf can you please add fixes #XXX when you open PRs, so that the issues get auto closed when the PR is merged? Thanks :) |
| @@ -378,7 +378,9 @@ object JsonCodec { | |||
|
|
|||
| private def enumEncoder[Z](schema: Schema.Enum[Z], cfg: Config, cases: Schema.Case[Z, _]*): ZJsonEncoder[Z] = | |||
| // if all cases are CaseClass0, encode as a String | |||
| if (schema.annotations.exists(_.isInstanceOf[simpleEnum])) { | |||
| if (schema.annotations.exists(_.isInstanceOf[simpleEnum]) && cases.forall( | |||
There was a problem hiding this comment.
since @simpleEnum is not added explicitly by the user, is this not an error in the macro?
And if the user would add it, the macro should fail.
But here, the user could have created a schema manually or added the annotation manually after the macro code.
So I think we should at least print a warning here, if the annotation is present but the cases are not parameter less. I would even prefer to fail.
There was a problem hiding this comment.
I had thought that it made sense to use simpleEnum if the final leaves were case objects, but actually that should not be the macro behaviour. I think that I will close this PR and add this to #707. I agree that it's better to make it fail.
Adding fixes from now on!
There was a problem hiding this comment.
@pablf I think it is okay, if all leaves are objects or case classes without fields, that it is a simple enum. But why would you need the check in the json codec for that? The check there makes sense to prevent errors in case of an annotation where no annotation belongs, to raise an error. But if leaves are simple cases and the json codec still can't handle it, it is a different error and this would not be the fix.
I think the goals should be
- simple leaves = simple enum
- simple leaves with intermediate traits should be handled by codecs as if the intermediate trait is missing
- no simple leaves but annotated => macro fails
- no simple leaves but annotated => codec fails
There was a problem hiding this comment.
Maybe we need more issues
There was a problem hiding this comment.
@987Nabil I've just noticed a different behaviour in this aspect between Scala 2 and Scala 3 macros. Scala 2 doesn't derives the intermediate traits in the schemas while Scala 3 does. Maybe that's the only issue here. Are intermediate traits supposed to appear in the schema or not? That would also solve the issue.
There was a problem hiding this comment.
I think it should not. At least I do not see any value in it. @jdegoes wdyt?
There was a problem hiding this comment.
@987Nabil Why not derive intermediate traits??
There was a problem hiding this comment.
I don't say it's not good. I say, is there a use case for it? Is there value in it? Or does it just make the use case of simple enum harder without a clear benefit.
Anyhow, if we have intermediate traits derived, it should be the same for Scala 2 and 3 and leave traits (no subtypes) should be ignored for the question of "Is this a simple enum".
There was a problem hiding this comment.
It seems like not deriving intermediates makes the system "lossy", which is something I like to avoid on principle even if I don't have a compelling use case for it.
There was a problem hiding this comment.
I am fine with that. Then the Scala 2 macro should do the same. And the structure should be interpreted correctly regarding simple enum
fixes #668
/claim #668