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

Revises the "union" method in SchemaVisitor ... #302

Merged
merged 5 commits into from
Jul 11, 2022

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Jul 11, 2022

... to make it easier to write memoised dispatching logic

The union method in SchemaVisitor is an obvious footgun, because it makes it a hard to memoise the compilation of schemas for each alternative.

This PR revises the union method to expose a new construct Alt.Dispatcher, which does the heavylifting of memoizing the result of compiling schemas, and dispatching the union instance to the correct alternative.

  • Creates a Alt.Dispatcher construct that aims at streamlining the
    implementation of the union methods in SchemaVisitors, which is the
    hardest due to the fact that the implementor has to think about memoization.
  • Creates an EncoderK typeclass (used by Alt.Dispatcher) to generalise
    over what is an encoder, and streamline further the implementation of
    union encoders.
  • Extract a generalisation of SchemaVisitor, named SchemaCompiler, which could
    be used to unify Schematic, which @kubukoz really really wants.
  • Examplifies the use of Alt.Dispatcher in DocumentEncoderSchemaVisitor

This should remove the need for #260

…e optimised dispatched logic

* Creates a `Alt.Dispatcher` construct that aims at streamlining the
implementation of the `union` methods in SchemaVisitors, which is the
hardest due to the fact that the implementor has to think about memoization.
* Creates an `EncoderK` typeclass (used by Alt.Dispatcher) to generalise
over what is an encoder, and streamline further the implementation of
union encoders.
* Extract a generalisation of SchemaVisitor, named SchemaCompiler, which could
be used to unify Schematic, which @kubukoz really really wants.
* Examplifies the use of `Alt.Dispatcher` in DocumentEncoderSchemaVisitor
def absorb[A](f: A => Document): DocumentEncoder[A] =
new DocumentEncoder[A] {
def apply: A => Document = f
override def canBeKey: Boolean = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is cheating a little but we're planning on removing that canBeKey stuff in a subsequent PR

import smithy.api.Http

object SchemaVisitorPathEncoder extends SchemaVisitor[MaybePathEncode] { self =>
Copy link
Contributor

Choose a reason for hiding this comment

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

good one

@daddykotex
Copy link
Contributor

This is a bit over my head, but I like it. We can eventually rewrite most of our implementation and get a free improvement with it

@Baccata
Copy link
Contributor Author

Baccata commented Jul 11, 2022

We can eventually rewrite most of our implementation and get a free improvement with it

There's not that many that actually encode things in this repo, and it might be a little bit less efficient than the raw management that the jsoniter interpreter does.

Comment on lines +47 to +53
trait Precompiler[F[_], G[_]] { self =>
def apply[A](label: String, instance: F[A]): G[A]
def toPolyFunction[U]: PolyFunction[Alt[F, U, *], G] =
new PolyFunction[Alt[F, U, *], G] {
def apply[A](fa: Alt[F, U, A]): G[A] = self.apply(fa.label, fa.instance)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: some helper for converting from F ~> G (e.g. if you want to simply lift a visitor to Precompiler) would be great (it'd just ignore the label)

Copy link
Member

Choose a reason for hiding this comment

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

tbh I don't actually have a need for this, so feel free to ignore

*
* Useful in particular when encoding unions
*/
trait EncoderK[F[_], Result] extends Contravariant[F] {
Copy link
Member

Choose a reason for hiding this comment

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

Would this be useful?

implicit def encoderKForFunction[B]: EncoderK[* => B, B] =
  new EncoderK[* => B, B] {
    def apply[A](fa: A => B, a: A): B = fa(a)

    def absorb[A](f: A => B): A => B = f
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, possibly, doesn't hurt to add.

@Baccata Baccata merged commit 97974ea into dfrancoeur/kill-schematic Jul 11, 2022
@Baccata Baccata deleted the revise-union-dispatch branch July 11, 2022 21:47
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