ipfs / kubo

An IPFS implementation in Go
https://docs.ipfs.tech/how-to/command-line-quick-start/
Other
15.9k stars 2.98k forks source link

Add/Cat interface #696

Closed jbenet closed 9 years ago

jbenet commented 9 years ago

In #664 coreunix.Cat now takes a path. It was nice when Cat and Add were symmetric. Maybe make Add return a path is the right thing to do? cc @whyrusleeping @briantigerchow

whyrusleeping commented 9 years ago

Add could return a path, but it just contains a hash... im fine with it. But its halfway misleading...

btc commented 9 years ago

At this stage, probably want to involve people unfamiliar with the API.

whyrusleeping commented 9 years ago

@travisperson how user friendly do you think coreunix.Cat and Add are?

btc commented 9 years ago

Got bit by the conversion. The key k returned by Add is now an invalid argument to Cat.

:beetle: :beetle: :beetle: :beetle:

175             if _, err := coreunix.Cat(n, path.Path(k)); err != nil {

07:08:17.328 DEBUG       path: Resolve: ' �x{��+YDŽP�= �36
                                                         C�,w(���5a resolver.go:24
07:08:17.328 DEBUG       path: given path element is not a base58 string.
 resolver.go:39

With the new signature, users must perform multiple conversions to satisfy the function. The UX has just taken steps backward. :cry:

This is the correct use of the new signature...

175             if _, err := coreunix.Cat(n, path.Path(k.String())); err != nil {

Now that I have on my user :tophat:, here's the solution.

Both Add and Cat must accept/return human-readable string types.

Cat must accept a string type and must correctly handle the output of Add (without casts).

Cat may be intelligent and handle the raw util.Key form if it can do so reliably. ("be lenient in what you accept from others")

k, err := coreunix.Add(n, r)
r, err := coreunix.Cat(n, k)
whyrusleeping commented 9 years ago

I agree with this. I think Cat should perform a set of checks on its input, make sure it contains a base58 encoded hash somewhere, if the user passes in a key, convert it appropriately, etc. I think path should also have FromString and FromKey constructors to aid in useablility

whyrusleeping commented 9 years ago

I addressed this in #702

Dentrax commented 2 years ago

I think coreunix.Cat is removed at the master branch. What's the alternative way to do cat() in current version? @whyrusleeping @btc

Stebalien commented 2 years ago

This issue is quite old. Please ask on https://discuss.ipfs.io and provide some more context.