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

WIP: Changes needed for --cid-base option in go-ipfs. #8

Closed
wants to merge 10 commits into from

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Sep 19, 2018

No description provided.

@kevina kevina force-pushed the kevina/multibase branch 2 times, most recently from abff5cc to 3d173cb Compare November 14, 2018 03:52
@ghost ghost assigned kevina Nov 22, 2018
@ghost ghost added the status/in-progress In progress label Nov 22, 2018
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

First pass, need to read go-ipfs side first to get the idea behind some of this more.


// apicid.Hash is a type to respesnt a CID in the API which marshals
// as a string
type Hash struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just type Hash string?

@@ -0,0 +1,61 @@
package apicid
Copy link
Member

Choose a reason for hiding this comment

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

api sounds a bit weird here, I'd probably just move this into cidenc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may sound weird but that is the exact idea I want to capture. This package present a form of the Cid to use in the API struct. The only reason it is here is because this type might need to be used outside of go-ipfs.

func extractCidString(p string) string {
segs := path.FromString(p).Segments()
v := segs[0]
if v == "ipfs" && len(segs) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

ipld is a valid prefix too. I also think that FromPath should be somehow integrated into CoreAPI or somewhere like that like that as having this package depend on go-path is going to hurt us eventually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is extract the CID from a string which can either be just a Cid or a ipld path. I was not aware that ipld can be used but will fix that. I am not super happy with the go-path dependency either. I am open to other ways to do this.

Copy link
Member

Choose a reason for hiding this comment

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

Doing this the way described in ipfs/kubo#5789 (review) should remove the need for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that the FromPath function doesn't strictly need to be here. It can be moved back into go-ipfs as only code in go-ipfs uses it for now.

@@ -150,3 +150,48 @@ func encode(base mb.Encoder, data []byte, strip bool) string {
}
return str
}

// ScanForCid scans bytes for anything resembling a CId. If one is
Copy link
Member

Choose a reason for hiding this comment

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

s/CId/Cid/

@kevina
Copy link
Contributor Author

kevina commented Dec 12, 2018

Note: This p.r is superseded by #10 but it has a lot of useful code in it that might still be useful to get in. I am closing but please don't delete the branch.

@kevina kevina closed this Dec 12, 2018
@ghost ghost removed the status/in-progress In progress label Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants