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

Add/Cat interface #696

Closed
jbenet opened this issue Jan 30, 2015 · 8 comments
Closed

Add/Cat interface #696

jbenet opened this issue Jan 30, 2015 · 8 comments

Comments

@jbenet
Copy link
Member

jbenet commented Jan 30, 2015

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

whyrusleeping commented Jan 30, 2015

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

@btc
Copy link
Contributor

btc commented Jan 30, 2015

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

@whyrusleeping
Copy link
Member

whyrusleeping commented Jan 30, 2015

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

@btc
Copy link
Contributor

btc commented Jan 30, 2015

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

🪲 🪲 🪲 🪲

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

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

whyrusleeping commented Jan 30, 2015

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

whyrusleeping commented Jan 31, 2015

I addressed this in #702

@Dentrax
Copy link

Dentrax commented Jan 24, 2022

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

@Stebalien
Copy link
Member

Stebalien commented Feb 4, 2022

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

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

No branches or pull requests

5 participants