RFC: coreapi.Dag #4471

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

@@ -38,6 +40,12 @@ type UnixfsAPI interface {
 	Ls(context.Context, Path) ([]*Link, error)
 }

+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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: coreapi.Dag #4471

RFC: coreapi.Dag #4471

magik6k commented Dec 8, 2017 •

edited

Loading

whyrusleeping Dec 8, 2017

whyrusleeping Dec 8, 2017

whyrusleeping Dec 12, 2017

whyrusleeping Dec 12, 2017

whyrusleeping Dec 12, 2017

magik6k commented Dec 20, 2017

ghost Dec 20, 2017

ghost Dec 20, 2017

whyrusleeping Dec 20, 2017

ghost Dec 20, 2017

whyrusleeping Dec 21, 2017

magik6k Dec 21, 2017

ghost Dec 21, 2017

ghost left a comment

magik6k commented Dec 22, 2017

whyrusleeping commented Dec 23, 2017

RFC: coreapi.Dag #4471

RFC: coreapi.Dag #4471

Conversation

magik6k commented Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

magik6k commented Dec 22, 2017

whyrusleeping commented Dec 23, 2017

magik6k commented Dec 8, 2017 •

edited

Loading