commands: switch object to CoreAPI #4643

magik6k · 2018-02-02T21:45:52Z

This PR wires CoreAPI into command utils and makes ipfs object * use the api.

Only changing ipfs object to keep this PR small, will open another PRs for other command groups after this one is merged.

magik6k · 2018-02-02T21:47:31Z

core/coreapi/interface/interface.go

+
+	// WithResolve is an option for ParsePath which when set to true tells
+	// ParsePath to also resolve the path
+	WithResolve(bool) options.ParsePathOption


I'm wondering if I should make it default to true. cc @lgierth

This should definitely not be a method on the CoreAPI interface (just a function, or maybe just a type type WithResolve bool). However, I notice we're already doing this all over the place. Any reason for it?

Anyways, do we ever actually need ResolvePath to resolve? Doesn't everything that takes a path do resolution internally just in case?

It's based on a concept called 'functional options' from this article.

TL;DR - this pattern let's us expand the options without breaking anything, it might be overkill for some methods, but I prefer consistency more. But yeah, a separate method for resolving string path makes sense too

Anyways, do we ever actually need ResolvePath to resolve? Doesn't everything that takes a path do resolution internally just in case?

The problem is that paths are (currently) immutable and if user wanted to re-use the resolved path (ipns can be slow), we'd need to return the resolved path, which would result in calls to some methods getting a bit messy.

Having an explicit resolve will also make go-ipfs-api implementation simpler.

It's based on a concept called 'functional options' from this article.

I like the pattern, I'm just objecting to attaching these functions to the interface itself instead of leaving them as free functions. It actually goes against the spirit of the pattern as it makes it harder to grow the API: adding new options will break existing implementations of the CoreAPI interface.

The problem is that paths are (currently) immutable and if user wanted to re-use the resolved path (ipns can be slow), we'd need to return the resolved path, which would result in calls to some methods getting a bit messy.

Fair enough. However, in this case (and many others) we know that we're only going to use the path once (as far as I can tell).

I like the pattern, I'm just objecting to attaching these functions to the interface itself instead of leaving them as free functions.

I feel exactly the same.

whyrusleeping

cursory review LGTM, would like one other +1 before merge

Stebalien · 2018-02-06T20:03:22Z

core/coreapi/interface/interface.go

+)
+
+// ObjectChange represents a change ia a graph
+// TODO: do we want this to be an interface?


TODO: Make a decision before merging (or, at least, file an issue).

Stebalien · 2018-02-06T20:07:52Z

core/coreapi/coreapi.go

@@ -112,16 +114,26 @@ type path struct {
 }

 // ParsePath parses path `p` using ipfspath parser, returns the parsed path.
-func ParsePath(p string) (coreiface.Path, error) {
+func (api *CoreAPI) ParsePath(ctx context.Context, p string, opts ...caopts.ParsePathOption) (coreiface.Path, error) {


The second this resolves the path, it's no longer parsing it. Maybe, instead, expose a ResolvePathString method (or just keep the steps separate).

Stebalien · 2018-02-06T20:12:38Z

core/coreapi/interface/interface.go

+
+	// WithResolve is an option for ParsePath which when set to true tells
+	// ParsePath to also resolve the path
+	WithResolve(bool) options.ParsePathOption


This should definitely not be a method on the CoreAPI interface (just a function, or maybe just a type type WithResolve bool). However, I notice we're already doing this all over the place. Any reason for it?

Anyways, do we ever actually need ResolvePath to resolve? Doesn't everything that takes a path do resolution internally just in case?

Stebalien · 2018-02-06T20:17:52Z

core/coreapi/coreapi.go

 }

 // ParseCid parses the path from `c`, retruns the parsed path.
-func ParseCid(c *cid.Cid) coreiface.Path {
+func (api *CoreAPI) ParseCid(c *cid.Cid) coreiface.Path {


I really think this should remain a function.

It's done that way to expose this function on the public API - which will be eventually implemented by things like go-ipfs-api, so we need an option to abstract this a bit.

Will there ever be alternative implementations of path parsing? I'd rather not have to implement ParseCid when I implement the core interface API.

magik6k · 2018-02-08T17:19:42Z

Blocked on #4672

Stebalien

Only a few nits. Other than that, negative diffstats give me such a warm and fuzzy feeling.

Stebalien · 2018-07-19T16:11:27Z

commands/request.go

+
+// NodeWithoutConstructing returns the underlying node variable
+// so that clients may close it.
+func (c *Context) NodeWithoutConstructing() *core.IpfsNode {


Is this used?

Stebalien · 2018-07-19T16:11:56Z

core/commands/env.go

@@ -18,6 +19,16 @@ func GetNode(env interface{}) (*core.IpfsNode, error) {
 	return ctx.GetNode()
 }

+// GetApi extracts CoreAPI instance from the environment.
+func GetApi(env interface{}) (coreiface.CoreAPI, error) {


Should probably take cmds.Environment (same thing but clearer).

Stebalien · 2018-07-19T16:16:40Z

core/coreapi/interface/object.go

+)
+
+// ObjectChange represents a change ia a graph
+// TODO: do we want this to be an interface?


I think a type is sufficient.

Stebalien · 2018-07-19T16:17:02Z

core/coreapi/interface/object.go

@@ -31,6 +31,38 @@ type ObjectStat struct {
 	CumulativeSize int
 }

+const (
+	// DiffAdd is a Type of ObjectChange where a link was added to the graph
+	DiffAdd = iota


Maybe use a newtype here?

Stebalien · 2018-07-19T16:17:16Z

core/commands/object/patch.go

-// GetNode extracts the node from the environment.
-func GetNode(env interface{}) (*core.IpfsNode, error) {
+// GetApi extracts CoreAPI instance from the environment.
+func GetApi(env interface{}) (coreiface.CoreAPI, error) {


Take an Environment.

magik6k · 2018-07-25T21:04:02Z

Fixed

whyrusleeping · 2018-07-29T05:53:22Z

rebased it since i messed it up by extracting things

License: MIT Signed-off-by: Łukasz Magiera <[email protected]>

ghost assigned magik6k Feb 2, 2018

ghost added the status/in-progress In progress label Feb 2, 2018

magik6k commented Feb 2, 2018

View reviewed changes

magik6k requested review from a user and Stebalien February 3, 2018 00:17

magik6k mentioned this pull request Feb 4, 2018

CoreAPI progress tracking issue #4498

Open

51 tasks

magik6k force-pushed the feat/coreapi-commands branch from 0d2dd19 to afb490b Compare February 4, 2018 14:43

whyrusleeping approved these changes Feb 5, 2018

View reviewed changes

Stebalien requested changes Feb 6, 2018

View reviewed changes

magik6k mentioned this pull request Feb 8, 2018

Think through the CoreAPI Path API and implementation. #4666

Closed

magik6k added the status/blocked Unable to be worked further until needs are met label Feb 8, 2018

Kubuxu self-requested a review March 11, 2018 02:45

magik6k added the topic/core-api Topic core-api label Mar 11, 2018

magik6k mentioned this pull request Mar 11, 2018

coreapi: Refactor options #4807

Merged

magik6k force-pushed the feat/coreapi-commands branch from afb490b to f271b09 Compare July 19, 2018 12:27

magik6k removed the status/blocked Unable to be worked further until needs are met label Jul 19, 2018

magik6k force-pushed the feat/coreapi-commands branch from f271b09 to 3c67617 Compare July 19, 2018 12:38

magik6k requested a review from Stebalien July 19, 2018 13:28

Stebalien reviewed Jul 19, 2018

View reviewed changes

magik6k mentioned this pull request Jul 25, 2018

ipfs object diff assumes links will have different names for the comparison #5278

Closed

magik6k force-pushed the feat/coreapi-commands branch 2 times, most recently from b09eecd to 5eaf5ad Compare July 25, 2018 20:56

Stebalien approved these changes Jul 26, 2018

View reviewed changes

Stebalien added RFM and removed status/in-progress In progress labels Jul 26, 2018

whyrusleeping force-pushed the feat/coreapi-commands branch from 5eaf5ad to f03680b Compare July 29, 2018 05:41

ghost assigned whyrusleeping Jul 29, 2018

ghost added the status/in-progress In progress label Jul 29, 2018

magik6k force-pushed the feat/coreapi-commands branch from f03680b to 62f986e Compare July 30, 2018 15:33

magik6k added 5 commits August 1, 2018 13:32

wire CoreAPI into request context

76a897c

License: MIT Signed-off-by: Łukasz Magiera <[email protected]>

coreapi: Pin option for Object.Put

7ac229e

License: MIT Signed-off-by: Łukasz Magiera <[email protected]>

coreapi: implement Object.Diff

432ab62

License: MIT Signed-off-by: Łukasz Magiera <[email protected]>

commands: switch object commands to CoreAPI

36c6fb3

License: MIT Signed-off-by: Łukasz Magiera <[email protected]>

object coreapi: Address review

60039f2

License: MIT Signed-off-by: Łukasz Magiera <[email protected]>

magik6k force-pushed the feat/coreapi-commands branch from 62f986e to 60039f2 Compare August 1, 2018 11:37

whyrusleeping merged commit 83f22d5 into master Aug 1, 2018

whyrusleeping deleted the feat/coreapi-commands branch August 1, 2018 23:21

ghost removed the status/in-progress In progress label Aug 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commands: switch object to CoreAPI #4643

commands: switch object to CoreAPI #4643

magik6k commented Feb 2, 2018

magik6k Feb 2, 2018

Stebalien Feb 6, 2018

magik6k Feb 7, 2018

magik6k Feb 7, 2018

Stebalien Feb 7, 2018

Stebalien Feb 7, 2018

Kubuxu Mar 11, 2018

whyrusleeping left a comment

Stebalien Feb 6, 2018

Stebalien Feb 6, 2018

Stebalien Feb 6, 2018

Stebalien Feb 6, 2018

magik6k Feb 7, 2018

Stebalien Feb 7, 2018

magik6k commented Feb 8, 2018

Stebalien left a comment

Stebalien Jul 19, 2018

Stebalien Jul 19, 2018

Stebalien Jul 19, 2018

Stebalien Jul 19, 2018

Stebalien Jul 19, 2018

magik6k commented Jul 25, 2018

whyrusleeping commented Jul 29, 2018

commands: switch object to CoreAPI #4643

commands: switch object to CoreAPI #4643

Conversation

magik6k commented Feb 2, 2018

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

whyrusleeping left a 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

Choose a reason for hiding this comment

magik6k commented Feb 8, 2018

Stebalien left a 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

magik6k commented Jul 25, 2018

whyrusleeping commented Jul 29, 2018