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 simpleEnum annotation #564

Merged
merged 10 commits into from
Sep 5, 2023
Merged

added simpleEnum annotation #564

merged 10 commits into from
Sep 5, 2023

Conversation

pablf
Copy link
Member

@pablf pablf commented May 24, 2023

/claim #506
Throws exception if used where not applicable and it is added automatically if it is indeed a simple Enum.

@googley42
Copy link
Contributor

googley42 commented May 24, 2023

@pablf so is the intention to automatically insert the simpleEnum into the derived schema? I think in the original issue I wanted to control the application of the annotation (ie make this manual process) so that users of downstream libraries like zio-dynamodb can choose the encoding they want (we generate DynamoDB codecs based on ZIO schemas) eg new projects may want the default tagged union encoding and projects that need to integrate with legacy databases say could use this new annotation.

@pablf
Copy link
Member Author

pablf commented May 24, 2023

You are right. In practice, JSONCodec always checked if it was a simple Enum, so it does not give any option. This is why I thought it was intended to be added automatically. Do I remove that part?

@pablf
Copy link
Member Author

pablf commented May 24, 2023

And if we included an automaticallyAdded: Boolean in the annotation's definition? What do you think?

@pablf
Copy link
Member Author

pablf commented May 24, 2023

Now it should pass the tests.
@googley42 I have added automaticallyAdded. If you want to use you can use @simpleEnum and it will generate a simpleEnum(false) annotation.

@googley42
Copy link
Contributor

@pablf so I think the original intention of the issue was just to create a standard annotation for use by downstream libraries - we already have one for zio-dynamodb - but the the thought was to standardise it. The fact that the JSon codec already defaults to this behaviour probably means that there is no extra work required for derive gen macro - what do you think @jdegoes ?

@pablf
Copy link
Member Author

pablf commented May 25, 2023

@googley42 That feature for the JSONCodec was done in #526. Maybe it would also be interesting for a Schema to know if it is a simple Enum, so each one of its codecs can use that information as desired, instead of checking in each codec if it is really a simple Enum. Also it would be possible to have different behaviours if the annotation was added by the derive gen macro or not.

Although with the last commit I did you could use @simpleEnum as @enumOfCaseObjects in your code and modify in zio-dynamodb/Codec.scala

private def isCaseObjectAnnotation(annotations: Chunk[Any]): Boolean =
    annotations.exists {
      case simpleEnum(false) => true
      case _                   => false
    }

so if it is added automatically (simpleEnum(true)) it does not do anything (I am not that familiar with zio-dynamodb so I might be wrong). You wouldn't need either to check if it is really a simple Enum in zio-dynamodb Codec as it is already checked in the derive gen macro.

@googley42
Copy link
Contributor

@pablf

Maybe it would also be interesting for a Schema to know if it is a simple Enum, so each one of its codecs can use that information as desired, instead of checking in each codec if it is really a simple Enum

Ah yes - I get this now - this is great idea and will simplify the codecs of all downstream libs!

The only thing I want to confirm is that will a manually created @simpleEnum override the automatically generated one? I suspect this is case and if so do you think it would be a good idea to have a unit test for this?

@pablf
Copy link
Member Author

pablf commented May 26, 2023

@googley42 Then great! Yes, that would be the expected behaviour. I have added the tests to ensure that it's like that in the future.

Copy link
Contributor

@googley42 googley42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and I think this a great piece of functionality - however I am not a zio-schema maintainer or have much knowledge around macros! you may want to get another review cc @zio/zio-schema

Copy link
Contributor

@googley42 googley42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@vigoo vigoo merged commit bc2c20e into zio:main Sep 5, 2023
12 checks passed
@pablf pablf deleted the simpleEnum branch October 9, 2023 16:13
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.

3 participants