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

adt trait for sealed hierarchies with common members #787

Merged
merged 10 commits into from
Feb 9, 2023
Merged

Conversation

lewisjkl
Copy link
Contributor

@lewisjkl lewisjkl commented Feb 3, 2023

Adds a new trait smithy4s.meta#adt that when applied to Union shapes will lead to different rendering. Namely, the rendering will take common mixins from members of the ADT and move them to the level of the sealed trait. More documentation and test case examples to come as the PR progresses.

TODO:

  • Add validator that will check for proper usage of the adt trait
  • More example usages in sampleSpecs/examples to check proper handling of edge cases
  • Add documentation

@@ -10,7 +10,7 @@ import smithy4s.schema.Schema.int
object OrderNumber extends Newtype[Int] {
val id: ShapeId = ShapeId("smithy4s.example", "OrderNumber")
val hints: Hints = Hints(
smithy.api.Default(smithy4s.Document.fromDouble(0.0d)),
smithy.api.Box(),
Copy link
Contributor

Choose a reason for hiding this comment

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

wut ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is from changing the smithy file in sampleSpecs over to use the smithy version 2 syntax directly. Prior it was a version 1 file being loaded which would have added these other traits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the "box" trait had been removed :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.. this has really confused me before and I honestly don't remember what is going on here since it seems to conflict their documentation. But, here is an example using smithy-model version 1.27.2 (the latest)
CleanShot 2023-02-06 at 09 37 29

@lewisjkl
Copy link
Contributor Author

lewisjkl commented Feb 7, 2023

Let me know if there are other example usages that you think would be helpful to add.

@lewisjkl lewisjkl marked this pull request as ready for review February 7, 2023 17:32
Copy link
Contributor

@daddykotex daddykotex left a comment

Choose a reason for hiding this comment

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

Great work, for me, the only missing piece for this PR is a bit of documentation to complement what we have for @adtMember in ADTs section of the docs

@lewisjkl
Copy link
Contributor Author

lewisjkl commented Feb 8, 2023

Added some documentation. As an aside, should we now deprecate the adtMember trait (with the deprecated smithy trait)? I am thinking we should since it does basically the same thing. Thoughts?

Additionally, there are some requirements that are added onto the structure shapes that the union targets:

1. The structures must NOT be the target of any other union, structure, etc. They can only be the target in the ONE union that is annotated with the `adt` trait.
2. The structures must contain at least one member.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that one is necessary, but happy to merge the PR as is and revise later

@Baccata Baccata merged commit bf1c17c into series/0.17 Feb 9, 2023
@Baccata Baccata deleted the adt-trait branch February 9, 2023 10:09
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