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

Add Edge Support to DSL #262

Open
jeremysears opened this issue Oct 12, 2018 · 1 comment
Open

Add Edge Support to DSL #262

jeremysears opened this issue Oct 12, 2018 · 1 comment

Comments

@jeremysears
Copy link
Collaborator

I've added this issue to track a discussion related to adding Edge support to the DSL. I'm happy to push up a PR, but I'd like to make sure it's in the style you would like to see. A few questions:

  1. Adding edge support will require a new Converter.forDomainEdge implicit. I believe this will require me to create a new DomainEdge marker trait that extends DomainRoot, for use in the Converter.forDomainEdge context bound:
  implicit def forDomainEdge[DomainType <: ModelEdge](
      implicit
      marshaller: Marshallable[DomainType],
      graph: Graph): Converter.Aux[DomainType, Edge] = new Converter[DomainType] {
    type GraphType = Edge
    def toDomain(e: Edge): DomainType = marshaller.toCC(e)
    def toGraph(dt: DomainType): Edge = AnonymousEdge(dt)
  }

I'm wondering if we should also add a DomainNode marker trait that extends DomainRoot, so that the context-bound of Converter.forDomainNode can be more specific. That would be a breaking change, which is why I bring it up. It seems like the right thing to do now, though.

  1. It seems like a lot of the things that are going in to NodeSteps that could be shared between NodeSteps and EdgeSteps, so I propose creating an ElementSteps base class and pushing much of what's in NodeSteps into ElementSteps. It probably still makes sense to leave NodeSteps there, even if it is empty, simply to fix the EndGraph type-parameter to Vertex, and give ourselves a future location for steps that are truly vertex-specific.

  2. I think some of the things in NodeSteps should really be in Steps, such as filterOnEnd and sideEffect, which could apply to any EndDomain (such as a selected tuple).

  3. Some of the new steps you've added to NodeSteps aren't using the implicit Constructor pattern that you were using earlier. I presume you're expecting that there will be an implicit to lift NodeSteps to its domain-specific subclass. Is there a reason why you're moving away from the Constructor pattern? Should we rely on the runtime conversion moving forward, or should we prefer the Constructor pattern?

@mpollmeier
Copy link
Owner

As you've figured, I only handle vertices so far, simply because that's all we need at the moment (at my work). Introducing breaking changes for keeping the code clean is not a problem.

1/2/3) yes that all makes sense

  1. the constructor pattern is adding quite some complexity and is only used where absolutely needed, i.e. Steps.map and Steps.flatMap, but nowhere in NodeSteps. Maybe we can even remove it there - I'd actually appreciate that, but am open to other suggestions.
    Thanks Jeremy!

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

No branches or pull requests

2 participants