-
Notifications
You must be signed in to change notification settings - Fork 17
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] add multibase binary #42
base: master
Are you sure you want to change the base?
Conversation
9a28053
to
1be98b8
Compare
if err != nil { | ||
return err | ||
} | ||
fmt.Println(base.Encode(fileData)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd lift this out and read the data (file/argument) into a common variable first.
Commands: []*cli.Command{ | ||
{ | ||
Name: "encode", | ||
ArgsUsage: "<base>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArgsUsage: "<base>", | |
ArgsUsage: "<base> [<data>]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, maybe make the base a flag? I.e., --base=xyz
? That way, we can set a reasonable default base (e.g., base32) so users can just call $(multibase encode "foobar")
and get the result they expect.
} | ||
|
||
if p != "" { | ||
fileData, err := ioutil.ReadFile(p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: stdin? I'd expect multibase encode < file > encoded
to work.
}, | ||
}, | ||
Action: func(context *cli.Context) error { | ||
if context.NArg() != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, multibase decode < data > output
would be nice to support.
Name: "transcode", | ||
ArgsUsage: "<new-base> <data>", | ||
Usage: "transcode multibase data", | ||
Action: func(context *cli.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. multibase transcode new-base < inp > out
👍 |
There is an IPFS version of this PR which we're going to focus on landing first: ipfs/kubo#8180 |
This is a draft of a multibase binary that does reading + writing from files as well as transcoding. It still leaves room for improvement. For example, the CLI library used here seems to care about the ordering of flags and arguments which causes problems for
multibase encode
which can take either an argument or a file name.It seems likely that if we get good UX here we'll just include the flow in go-ipfs as a
ipfs multibase
subcommand.cc @lidel @Stebalien also feel free to make modifications/comments here