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 a typed link node to allow traversal with code gen'd builders across links #41

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

hannahhoward
Copy link
Collaborator

Goals

Mostly, I want to support fast Graphsync :)
But concretely, when performing a traversal, if my schema contains a hint about what the node on the other side of the link is, and I have a custom builder for that type, I should utilize that information to determine what node builder to use in NodeBuilderChooser.

This also unlocks a lot of customization of NodeBuilderChooser without having to modify the default function.

There will still need to be some graphsync work to handle starting with the right root node builder, but this feels like it would unlock a lot of behaviors for code-gen

For Discussion

I explored putting this method on the generated link itself. This turned out to have several problems -- specifically, links vary according to how they reference their data and how they load that data. Currently, the only link type is CIDs. It turned out to not be able to continue supporting an abstract link type and a concrete CID link type while also varying a link type according to where it is typed.

Add the concept of a suggested node builder to links that are a reference to a specific typed node
that has a code-gen'd node builder
@codecov
Copy link

codecov bot commented Jan 30, 2020

Codecov Report

Merging #41 into master will increase coverage by 0.04%.
The diff coverage is 83.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   72.21%   72.24%   +0.04%     
==========================================
  Files          50       50              
  Lines        3162     3173      +11     
==========================================
+ Hits         2283     2292       +9     
- Misses        789      790       +1     
- Partials       90       91       +1
Impacted Files Coverage Δ
schema/gen/go/genKindLinkNode.go 100% <100%> (ø) ⬆️
schema/gen/go/gen.go 100% <100%> (ø) ⬆️
traversal/common.go 77.78% <33.34%> (-9.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 907fb14...509995a. Read the comment docs.

Copy link
Collaborator

@warpfork warpfork left a comment

Choose a reason for hiding this comment

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

Yeah, this looks about right. Cool, thanks.

One minor comment that I think might be a typo of sorts, but otherwise I'd say go ahead and merge it.

I also tried to game it out putting the new method on the Link interface, but agree, it came out weird. It's correct that what kind of node to build may depend on what kind of structure we're already building (e.g., using codegen types vs not) and so it makes sense for this feature to be on the Node moreso than the Link.

I wonder what this will look like after NodeStyle from the research branch is introduced -- I bet this method might jump over to that type. And in doing so, might be able to remove the typed package from dependencies of traversal again, which seems like it might be a good thing. But that's a thing to sort out later; might as well merge this now.

@@ -98,6 +98,16 @@ func (gk generateKindLink) EmitTypedNodeMethodRepresentation(w io.Writer) {
`, w, gk)
}

func (gk generateKindLink) EmitTypedLinkNodeMethodReferencedLinkBuilder(w io.Writer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/ReferencedLinkBuilder/ReferencedNodeBuilder/ ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

awesome, done, and then I'm gonna merge

rename EmitTypedLinkNodeMethodReferencedLinkBuilder to EmitTypedLinkNodeMethodReferencedNodeBuilder
@hannahhoward hannahhoward merged commit 80e0a48 into master Jan 30, 2020
@warpfork warpfork deleted the feat/typed-link-node branch February 27, 2020 12:06
@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
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.

2 participants