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

Support enum encoding #307

Open
wants to merge 2 commits into
base: series/1.x
Choose a base branch
from
Open

Support enum encoding #307

wants to merge 2 commits into from

Conversation

jgoday
Copy link
Contributor

@jgoday jgoday commented May 15, 2021

From #105.

It allows to encode/decode enums (as sealed traits and case objects) like

   sealed trait Color
   case object Green extends Color
   case object Yellow extends Color

I think that some things needs some revision or maybe a different/better solution:

1 - When decoding a SealedTrait, lexer tries to consumes '{', if it throws an UnsafeJson exception, we try to match an enum value.
2 - Find a better approach to determine if an object is a case object (should work in scala.js too)

def isCaseObject(x: Any): Boolean = x.getClass.getName.endsWith("$")

@jgoday jgoday mentioned this pull request May 15, 2021
@fsvehla
Copy link
Contributor

fsvehla commented May 16, 2021

@fommil I’d appreciate your opinion here.

@fommil
Copy link
Contributor

fommil commented May 16, 2021

conceptually this is nice but it is too magical for my personal tastes. I'd rather this was its own macro. Firstly it breaks backwards compatibility, and secondly it means that your entire sealed trait encoding can change just because you add a non case object.

@jgoday
Copy link
Contributor Author

jgoday commented May 17, 2021

conceptually this is nice but it is too magical for my personal tastes. I'd rather this was its own macro. Firstly it breaks backwards compatibility, and secondly it means that your entire sealed trait encoding can change just because you add a non case object.

I agree, especially regarding the backwards compatibility and the lexer/exceptions part.
It was just an attempt to implement that use case and experimenting with the existing codebase.
Please, remove the PR if necessary.

@fsvehla
Copy link
Contributor

fsvehla commented May 17, 2021

@jgoday If we implement this with another macro, we could keep compatibility and fail compilation if not every member of the sealed hierarchy is a case object — wdyt?

@jgoday
Copy link
Contributor Author

jgoday commented May 18, 2021

@jgoday If we implement this with another macro, we could keep compatibility and fail compilation if not every member of the sealed hierarchy is a case object — wdyt?

@fsvehla Sounds good, what should it be called (instead of DeriveJsonDecoder) ?

@fsvehla
Copy link
Contributor

fsvehla commented May 19, 2021

@jgoday DeriveJsonDecoderEnum?

@jgoday
Copy link
Contributor Author

jgoday commented May 21, 2021

@fsvehla, I have implemented it like this jgoday@2f7160a

Two different macros (DeriveJsonDecoderEnum/DeriveJsonEncoderEnum) only for the sealed traits/case objects like enums.

Is this a valid approach ? If so, we should implement toJsonAST/fromJsonAST tests and keep running existing annotations with the new encoder/decoder for enums.

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.

None yet

3 participants