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

Improve paths in CoreApi #4672

Merged
merged 15 commits into from
Jul 17, 2018
Merged

Improve paths in CoreApi #4672

merged 15 commits into from
Jul 17, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Feb 8, 2018

Fixes #4666

TODO:

@Stebalien
Copy link
Member

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

@magik6k magik6k added the topic/core-api Topic core-api label Mar 11, 2018
@Kubuxu 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 magik6k requested a review from Kubuxu as a code owner March 26, 2018 10:15
@ghost ghost added the status/in-progress In progress label Mar 26, 2018
@magik6k magik6k force-pushed the feat/coreapi-paths branch 2 times, most recently from f71e588 to 0ad0aed Compare March 30, 2018 20:49
@magik6k magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 30, 2018
@magik6k
Copy link
Member Author

magik6k commented Apr 3, 2018

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

@magik6k magik6k added the need/review Needs a review label Apr 3, 2018
@Kubuxu Kubuxu removed the need/review Needs a review label Apr 17, 2018

// 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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the motivation?

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Kubuxu commented Apr 19, 2018

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

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some docs

return p.(coreiface.ResolvedPath), nil
}

ipath := p.(*path).path
Copy link
Member

Choose a reason for hiding this comment

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

This cast shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

return nil, err
}

resolveOnce := uio.ResolveUnixfsOnce
Copy link
Member

Choose a reason for hiding this comment

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

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
}

}

}

func (p *path) Namespace() string {
if len(p.path.Segments()) < 1 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Won't all paths have namespaces now?

They do, ipfspath.ParsePath guarantees that.

Copy link
Member

Choose a reason for hiding this comment

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

Could we panic here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@magik6k
Copy link
Member Author

magik6k commented Apr 24, 2018

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

@magik6k magik6k changed the title [RFC] Improve paths in CoreApi Improve paths in CoreApi May 17, 2018
@magik6k
Copy link
Member Author

magik6k commented May 19, 2018

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

@ghost ghost added the status/in-progress In progress label Jun 27, 2018
@magik6k
Copy link
Member Author

magik6k commented Jul 3, 2018

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

@whyrusleeping
Copy link
Member

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

@whyrusleeping
Copy link
Member

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

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

magik6k commented Jul 17, 2018

done

@Stebalien Stebalien removed the status/in-progress In progress label Jul 17, 2018
@whyrusleeping whyrusleeping merged commit 9bad5fe into master Jul 17, 2018
@whyrusleeping whyrusleeping deleted the feat/coreapi-paths branch July 17, 2018 23:07
@magik6k magik6k mentioned this pull request Oct 31, 2018
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
Improve paths in CoreApi

This commit was moved from ipfs/kubo@9bad5fe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/community-input Needs input from the wider community RFM topic/core-api Topic core-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants