RFC: coreapi.Dag by magik6k · Pull Request #4471 · ipfs/kubo

magik6k · 2017-12-08T22:03:08Z

TODO:

Review
Docs
Tests

whyrusleeping · 2017-12-08T22:08:09Z

core/coreapi/dag.go

+type DagAPI CoreAPI
+
+func (api *DagAPI) Add(ctx context.Context, src io.Reader, inputEnc string, format cid.Prefix) ([]coreiface.Node, error) {
+	nds, err := coredag.ParseInputs(inputEnc, cid.CodecToStr[format.Codec], src, format.MhType, format.MhLength)


we should probably validate format.Codec, otherwise we would return bad data from the cid.CodeToStr map

whyrusleeping · 2017-12-08T22:13:02Z

core/coreapi/interface/interface.go

 }

+type DagAPI interface {
+	Add(ctx context.Context, src io.Reader, inputEnc string, format cid.Prefix) ([]Node, error)


Maybe Put instead of Add?

I also think its important for people to be able to call these methods without having to import other packages. So making the format argument a pointer, and having some sort of defaults for when the user passes nil is something we should think about.

whyrusleeping · 2017-12-12T00:31:18Z

core/coreapi/dag.go

+	return api.core().ResolveNode(ctx, path)
+}
+
+func (api *DagAPI) Tree(ctx context.Context, p coreiface.Path, depth int) ([]coreiface.Path, error) {


this makes me realize we should have an ipfs dag tree command

whyrusleeping · 2017-12-12T00:32:06Z

core/coreapi/dag_test.go

+		t.Error(err)
+	}
+
+	_, err = api.Dag().Put(ctx, strings.NewReader(`"Hello"`), "json", nil)


I would check the Cid of the created object. Just one more additional protection against us breaking things in the future.

whyrusleeping · 2017-12-12T00:35:00Z

core/coreapi/interface/interface.go

+
 	ResolvePath(context.Context, Path) (Path, error)
-	ResolveNode(context.Context, Path) (Node, error)
+	ResolveNode(context.Context, Path) (Node, error) //TODO: should this get dropped in favor of DagAPI.Get?


I think this is fine to keep as a helper alias

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k · 2017-12-20T14:44:49Z

This is the first PR in the coreapi series that needs to get merged, mostly due to test utils being done here.

@whyrusleeping do you see anything more TODO here?

ghost · 2017-12-20T15:13:29Z

core/coreapi/interface/interface.go

+// DagAPI specifies the interface to IPLD
+type DagAPI interface {
+	// Put inserts data using specified format and input encoding.
+	// If format is not specified (nil), default dag-cbor/sha256 is used


Unless used with WithCodec or WithHash, the defaults "dag-cbor" and "sha256" are used.

ghost · 2017-12-20T15:15:25Z

core/coreapi/interface/interface.go

+type DagAPI interface {
+	// Put inserts data using specified format and input encoding.
+	// If format is not specified (nil), default dag-cbor/sha256 is used
+	Put(ctx context.Context, src io.Reader, opts ...options.DagPutOption) ([]Node, error)


Why is it desirable to have Put() return a slide of Nodes?

I guess it could return a slice of cids instead. But we need to know what was added.

I mean why is it desirable that it's a slice -- isn't there a root node for the added dag?

hrm... I think all of our current usecases would be fine with just the 'root' cid. Can you confirm that @magik6k ?

It will be fine, yes. I decided to return a slice as it is what was returned by coredag.ParseInputs, I think the reasoning behind it was that the reader can provide more than one node, though it's not implemented now.

Mh okay but what's the scenario that makes us want a slice returned here, versus just calling Tree() with the Node.Cid() returned from Put()? We'll keep all those nodes around in memory to return them from a function whose purpose isn't to iterate a graph (that's what Tree() is for).

I feel like we can rather be consistent with js-ipfs here.

ghost

Two tiny comments, LGTM otherwise 👍

magik6k · 2017-12-22T15:45:59Z

Made Dag.Put return a single node, this makes more sense than a slice. When we add a batching interface here in the future, having this method return a slice would be redundant (plus this wasn't really a batching interface..).

whyrusleeping · 2017-12-23T00:19:45Z

@magik6k any idea why the tests are all failing?

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k requested a review from a user December 8, 2017 22:03

ghost assigned magik6k Dec 8, 2017

ghost added the status/in-progress In progress label Dec 8, 2017

whyrusleeping reviewed Dec 8, 2017

View reviewed changes

magik6k force-pushed the feat/coreapi/dag branch from 13b7adb to c780466 Compare December 8, 2017 22:10

whyrusleeping reviewed Dec 8, 2017

View reviewed changes

magik6k force-pushed the feat/coreapi/dag branch from c780466 to 0a9cf52 Compare December 8, 2017 22:25

magik6k added the topic/core-api Topic core-api label Dec 10, 2017

magik6k force-pushed the feat/coreapi/dag branch from 53a14fd to 34fe6d0 Compare December 11, 2017 16:26

whyrusleeping reviewed Dec 12, 2017

View reviewed changes

magik6k force-pushed the feat/coreapi/dag branch 3 times, most recently from 753540a to 7b5b4f1 Compare December 14, 2017 23:14

magik6k mentioned this pull request Dec 15, 2017

CoreAPI progress tracking issue #4498

Open

51 tasks

magik6k force-pushed the feat/coreapi/dag branch from 7b5b4f1 to 11bc903 Compare December 17, 2017 00:11

magik6k added 2 commits December 20, 2017 13:44

coreapi: DAG API proposal

82924fb

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: add tests for dag

b40a6f8

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/coreapi/dag branch 3 times, most recently from 4b62309 to b9be6c1 Compare December 20, 2017 12:57

coreapi: functional options for DagAPI

f153c01

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/coreapi/dag branch from b9be6c1 to f153c01 Compare December 20, 2017 14:03

ghost reviewed Dec 20, 2017

View reviewed changes

ghost approved these changes Dec 20, 2017

View reviewed changes

magik6k force-pushed the feat/coreapi/dag branch from a3952e2 to 460e0ae Compare December 22, 2017 15:39

magik6k mentioned this pull request Dec 22, 2017

RFC: coreapi.Name,Key #4477

Merged

3 tasks

magik6k force-pushed the feat/coreapi/dag branch from 460e0ae to f213060 Compare December 22, 2017 16:28

coreapi: dag review

db31833

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/coreapi/dag branch from f213060 to db31833 Compare December 23, 2017 03:49

whyrusleeping merged commit 952a143 into master Dec 31, 2017

whyrusleeping deleted the feat/coreapi/dag branch December 31, 2017 20:58

ghost removed the status/in-progress In progress label Dec 31, 2017

Uh oh!

Conversation

magik6k commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

magik6k commented Dec 22, 2017

Uh oh!

whyrusleeping commented Dec 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

magik6k commented Dec 8, 2017 •

edited

Loading