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

Override gen annotation #446

Closed
wants to merge 1 commit into from
Closed

Conversation

beem812
Copy link
Contributor

@beem812 beem812 commented Nov 24, 2022

@generator() annotation for specifying a different generator for the DeriveGen functionality to use. #438

@beem812 beem812 requested a review from a team as a code owner November 24, 2022 15:08
@CLAassistant
Copy link

CLAassistant commented Nov 24, 2022

CLA assistant check
All committers have signed the CLA.

@beem812 beem812 force-pushed the override-gen-annotation branch from 1ea21a7 to 65def1c Compare November 24, 2022 15:11
@@ -109,7 +109,8 @@ lazy val zioSchema = crossProject(JSPlatform, JVMPlatform)
libraryDependencies ++= Seq(
"dev.zio" %% "zio" % zioVersion,
"dev.zio" %% "zio-streams" % zioVersion,
"dev.zio" %% "zio-prelude" % zioPreludeVersion
"dev.zio" %% "zio-prelude" % zioPreludeVersion,
"dev.zio" %% "zio-test" % zioVersion
Copy link
Contributor Author

@beem812 beem812 Nov 24, 2022

Choose a reason for hiding this comment

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

I had to include this in the zio-schema project for the annotation definition, would it make sense for this annotation to live inside the zio-schema-zio-test project to avoid including this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes!

Potentially working needs cleanup

Add test to show it ignores a generator of a different type

Remove type safety and test

Formatting and test name fix

Formatting and linting fixes

formatting

Formatting again
@beem812 beem812 force-pushed the override-gen-annotation branch from 9306d79 to e7b60e7 Compare November 24, 2022 23:17
@jdegoes
Copy link
Member

jdegoes commented Dec 7, 2022

@vigoo Can we solve this problem by using the new type class derivation mechanism?

@vigoo
Copy link
Contributor

vigoo commented Dec 7, 2022

If DeriveGen would be rewritten as a Deriver it would mean that:

  • It can pick up custom implicit Gens for a type
  • That could be used to provide a custom Gen[String] for example

The annotation solution proposed in this PR is a bit more powerful because it allows customizing separate generators per usage within a single derivation, while with the implicit based you could just overwrite it once (but it could achieve the same thing by introducing multiple scoped implicits and expicitly deriving the Gen for subtrees).

@@ -0,0 +1,5 @@
package zio.schema.annotation
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to the zio-schema-zio-test module?

@jdegoes
Copy link
Member

jdegoes commented Dec 7, 2022

@vigoo All right, sounds like there may be a use case for this, then! Thanks.

@beem812 Just one comment, then good to merge after conflicts are fixed and build is passing. Thanks for your work on this!

@vigoo
Copy link
Contributor

vigoo commented Dec 7, 2022

@jdegoes anyway, rewriting DeriveGen with Deriver is a good idea (as a separate ticket) as it would enable the use of custom generators

@jdegoes
Copy link
Member

jdegoes commented Dec 8, 2022

@beem812 Happy to get this in. Please just re-open when conflicts are fixed and revisions are complete. Thank you!

@jdegoes jdegoes closed this Dec 8, 2022
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.

5 participants