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

Implement a simple SchemaDescription visitor #295

Merged
merged 13 commits into from
Jul 11, 2022
Merged

Implement a simple SchemaDescription visitor #295

merged 13 commits into from
Jul 11, 2022

Conversation

daddykotex
Copy link
Contributor

I'm not sure this is what was expected, mostly because I figure the solution is much more verbose than the current solution. On the other side, this visitor will be available for other usages if we need it.

@daddykotex daddykotex requested a review from Baccata July 7, 2022 13:04
@daddykotex daddykotex marked this pull request as draft July 7, 2022 18:36
@daddykotex
Copy link
Contributor Author

I'll mark this as draft, I'm not able to implement lazy with the Set[ShapeId] => (Set[ShapeId], String) approach

@daddykotex daddykotex marked this pull request as ready for review July 8, 2022 15:32
@daddykotex
Copy link
Contributor Author

Ready for review I guess

Copy link
Contributor

@Baccata Baccata left a comment

Choose a reason for hiding this comment

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

A few nitpicks that I'd like addressed before merging, lgtm otherwise

@daddykotex daddykotex marked this pull request as draft July 8, 2022 17:33
@daddykotex
Copy link
Contributor Author

Remarking as draft, I just merged into my other branch and I'm not satisfied. Everything works fine, but the Set[ShapeId] is leaking out and it should not

@daddykotex daddykotex marked this pull request as ready for review July 8, 2022 18:30
daddykotex added a commit that referenced this pull request Jul 8, 2022
@@ -42,4 +42,27 @@ trait SchemaVisitor[F[_]] extends (Schema ~> F) {
case SurjectionSchema(schema, to, from) => surject(schema, to, from)
case LazySchema(make) => lazily(make)
}

def mapK[G[_]](f: F ~> G): SchemaVisitor[G] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

To remove. Schema ~> F already has andThen which achieves exactly the same thing.

https://github.com/disneystreaming/smithy4s/blob/main/modules/core/src/smithy4s/PolyFunction.scala#L28-L31

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in 73ec53e

@Baccata Baccata merged commit 73df266 into main Jul 11, 2022
@Baccata Baccata deleted the dfrancoeur/desc branch July 11, 2022 07:57
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

2 participants