Improve paths in CoreApi #4672

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

Fixes #4666

TODO:

Path remainders
Mutable vs Immutable paths
- Just path.Namespace() == "ipns" or something more complex?
More things from Think through the CoreAPI Path API and implementation. #4666

Stebalien · 2018-02-08T20:32:50Z

Responded to the TODOs here: #4666 (comment). However, all that will take a while...

magik6k · 2018-04-03T13:04:08Z

I imported suggestions from #4666, @Stebalien can you have a look? (start from interface changes)

Stebalien · 2018-04-19T07:15:25Z

core/coreapi/interface/coreapi.go


 	// ResolveNode resolves the path (if not resolved already) using Unixfs
 	// resolver, gets and returns the resolved Node
 	ResolveNode(context.Context, Path) (ipld.Node, error)
+
+	// ParsePath parses string path to a Path
+	ParsePath(string) (Path, error)


I'm not really a fan of attaching helper functions to instance objects. What's the motivation?

Although... that could allow us to put a Resolve method on the path object itself...

What's the motivation?

We need ResolvePath on the interface as it is quite implementation specific, I thrown the other methods here for consistency.

I agree with ResolvePath. However ParsePath, IpfsPath, and IpldPath seem like pure helper functions.

(I don't feel that strongly, it just feels a bit funny).

So, I'm starting to feel more strongly about this. The core API should be as small as we can make it. Sticking helper functions into the API (especially ones like IpfsPath and IpldPath) will force us to keep extending the API every time we need a new helper function). Unless there's a really good reason to do this, I'd like to remove them.

Kubuxu · 2018-04-19T11:53:29Z

@magik6k it looks like test coverage of core api is low in general. Could you work on this in future?

Stebalien

This is going to make so much of our code so much cleaner.

Stebalien · 2018-04-19T13:43:16Z

core/coreapi/interface/path.go

+
+// ResolvedPath is a resolved Path
+type ResolvedPath interface {
+	// Cid returns the CID referred to by path


This (Cid versus Root) will need some careful documentation.

Added some docs

Stebalien · 2018-04-19T13:45:58Z

core/coreapi/path.go

+		return p.(coreiface.ResolvedPath), nil
+	}
+
+	ipath := p.(*path).path


This cast shouldn't be necessary.

This needs to be here to get to internal ipfspath.Path, at least for now.

I'll deduplicate logic from here and ipfspath in a separate PR, doing so in this PR would make it way bigger than it needs to be.

Stebalien · 2018-04-19T13:53:30Z

core/coreapi/path.go

+		return nil, err
+	}
+
+	resolveOnce := uio.ResolveUnixfsOnce


Given that we now have the namespace, we can pick the resolver based on it (possibly even create a mapping). We can do the mapping later but, for now, we should do something like:

var resolveOnce ResolveOnce switch path.Namespace() { case "ipfs": resolveOnce = uio.ResolveUnixfsOnce case "ipld": resolveOnce = resolver.ResolveSingle default: return someErr }

}

Stebalien · 2018-04-19T14:05:51Z

core/coreapi/path.go

+}
+
+func (p *path) Namespace() string {
+	if len(p.path.Segments()) < 1 {


Won't all paths have namespaces now? It would be nice to check this (and parse out the namespace) on initial parse.

Won't all paths have namespaces now?

They do, ipfspath.ParsePath guarantees that.

Could we panic here?

I pushed just that: https://github.com/ipfs/go-ipfs/pull/4672/files#diff-aec5b488acbd54adbfbc67351fc51d7aR140

magik6k · 2018-04-24T14:53:14Z

I've added some more tests and fixed rest of the stuff that needed fixing

magik6k · 2018-05-19T13:45:45Z

@Stebalien @Kubuxu can you have a look at this PR, would be great to have this move (it blocks most of coreapi-related efforts)

magik6k · 2018-07-03T16:01:23Z

@whyrusleeping can we try to get this in after 0.4.16 release stuff is done?

whyrusleeping · 2018-07-05T20:59:39Z

@magik6k Yes! Getting the other coreAPI work unblocked is high priority. Sorry for the lag

whyrusleeping · 2018-07-17T04:44:38Z

@magik6k alright, mind rebasing this? This will be the next thing we merge

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

magik6k · 2018-07-17T07:16:33Z

done

Improve paths in CoreApi This commit was moved from ipfs/kubo@9bad5fe

ghost assigned magik6k Feb 8, 2018

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

magik6k mentioned this pull request Feb 8, 2018

commands: switch object to CoreAPI #4643

Merged

magik6k requested review from a user and Stebalien February 8, 2018 17:20

Stebalien mentioned this pull request Feb 8, 2018

Think through the CoreAPI Path API and implementation. #4666

Closed

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

magik6k mentioned this pull request Mar 12, 2018

[WIP] Implement CoreAPI ipfs/go-ipfs-api#76

Closed

4 tasks

Kubuxu added need/community-input Needs input from the wider community status/blocked Unable to be worked further until needs are met and removed status/in-progress In progress labels Mar 15, 2018

magik6k force-pushed the feat/coreapi-paths branch from 87cb6a2 to 4598c0e Compare March 26, 2018 10:15

magik6k requested a review from Kubuxu as a code owner March 26, 2018 10:15

ghost added the status/in-progress In progress label Mar 26, 2018

magik6k force-pushed the feat/coreapi-paths branch 2 times, most recently from f71e588 to 0ad0aed Compare March 30, 2018 20:49

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

magik6k added the need/review Needs a review label Apr 3, 2018

Kubuxu removed the need/review Needs a review label Apr 17, 2018

Stebalien reviewed Apr 19, 2018

View reviewed changes

magik6k mentioned this pull request Apr 21, 2018

[WIP] Next round of extractions (blockservice, merkledag, path) #4960

Closed

magik6k force-pushed the feat/coreapi-paths branch from f95f862 to 2dd511f Compare April 21, 2018 21:32

magik6k mentioned this pull request May 4, 2018

[WIP] feat(dag): add IPLD to dag.get ipfs-inactive/js-ipfs-http-client#755

Open

magik6k changed the title ~~[RFC] Improve paths in CoreApi~~ Improve paths in CoreApi May 17, 2018

ghost added the status/in-progress In progress label Jun 27, 2018

magik6k added 14 commits July 17, 2018 08:40

coreapi: expand public path api

338e90e

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

coreapi: move path impl to path.go

4f6c066

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

coreapi: separate path into two types

15f8fc6

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

coreapi: remove ctx from ParsePath, split ParseCid

f5f44ab

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

coreapi: path.Mutable

7ee6194

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

coreapi: path remainders

9b28856

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

coreapi: path review

1d23bbe

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

coreapi: add more docs for path

d5b5e0d

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

path: add tests for ipld paths

604b187

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

coreapi: fix ipns path resolving

e227fc3

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

coreapi: fix resolved path root for some paths

0f6bd2d

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

coreapi: fix TestGatewayGet after rebase

0adb69a

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

coreapi: more docs for ResolvedPath

082498d

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

coreapi: move path utils to interface

7adf1cb

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

magik6k force-pushed the feat/coreapi-paths branch from be180c1 to 9d6e786 Compare July 17, 2018 06:47

coreapi: path rebase

4d5a96b

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

magik6k force-pushed the feat/coreapi-paths branch from 9d6e786 to 4d5a96b Compare July 17, 2018 07:00

Stebalien removed the status/in-progress In progress label Jul 17, 2018

whyrusleeping merged commit 9bad5fe into master Jul 17, 2018

whyrusleeping deleted the feat/coreapi-paths branch July 17, 2018 23:07

This was referenced Jul 21, 2018

Resolving links in sharded directories does not seem to work #5270

Closed

Release 0.4.17 #5267

Merged

magik6k mentioned this pull request Sep 20, 2018

Cleanup instances of manual resolver construction #5492

Merged

magik6k mentioned this pull request Oct 31, 2018

prepare for RC2 #5702

Merged

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#4672 from ipfs/feat/coreapi-paths

40666eb

Improve paths in CoreApi This commit was moved from ipfs/kubo@9bad5fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve paths in CoreApi #4672

Improve paths in CoreApi #4672

magik6k commented Feb 8, 2018 •

edited

Loading

Stebalien commented Feb 8, 2018

magik6k commented Apr 3, 2018

Stebalien Apr 19, 2018

Stebalien Apr 19, 2018

magik6k Apr 19, 2018

Stebalien Apr 19, 2018

Stebalien Jun 11, 2018

Kubuxu commented Apr 19, 2018

Stebalien left a comment

Stebalien Apr 19, 2018

magik6k Apr 20, 2018

Stebalien Apr 19, 2018

magik6k Apr 20, 2018

Stebalien Apr 20, 2018

Stebalien Apr 19, 2018

Stebalien Apr 19, 2018

magik6k Apr 20, 2018

Stebalien Apr 20, 2018

magik6k Apr 20, 2018

magik6k commented Apr 24, 2018

magik6k commented May 19, 2018 •

edited

Loading

magik6k commented Jul 3, 2018

whyrusleeping commented Jul 5, 2018

whyrusleeping commented Jul 17, 2018

magik6k commented Jul 17, 2018

Improve paths in CoreApi #4672

Improve paths in CoreApi #4672

Conversation

magik6k commented Feb 8, 2018 • edited Loading

Stebalien commented Feb 8, 2018

magik6k commented Apr 3, 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

Kubuxu commented Apr 19, 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

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 Apr 24, 2018

magik6k commented May 19, 2018 • edited Loading

magik6k commented Jul 3, 2018

whyrusleeping commented Jul 5, 2018

whyrusleeping commented Jul 17, 2018

magik6k commented Jul 17, 2018

magik6k commented Feb 8, 2018 •

edited

Loading

magik6k commented May 19, 2018 •

edited

Loading