Provide new "cid" sub-command. #5385

kevina · 2018-08-15T23:15:16Z

Implements #5357.

~~Still needs tests.~~

Closes #5229. Closes #5357.

Stebalien · 2018-08-24T22:30:53Z

core/commands/cid.go

+` + cidutil.FormatRef,
+	},
+	Arguments: []cmdkit.Argument{
+		cmdkit.StringArg("cid", true, true, "Cids to format."),


Can we use WithDefault("%s")?

You meant for the -f option? If it is omitted that is the default (see below), I just didn't use WithDefault. The idea was the -f is optional and if its not specified it will just return the CIDs.

It's nice from a documentation standpoint.

No Problem.

Stebalien · 2018-08-24T22:31:38Z

core/commands/cid.go

+		case "":
+			opts.fmtStr = "%s"
+		case "prefix":
+			opts.fmtStr = "%P"


This feels a bit magical. What's the motivation? We could also just add a ipfs cid prefix command (alias of ipfs cid format -f '%P' ...).

It was there in the original cid-fmt utility and copied over here. For the cid-fmt utility is was the original use case (to get information about cids). I am not tied to keeping it in this utility.

I'd remove it for now. We can consider either a prefix or a show command later but we probably don't need them now.

Stebalien · 2018-08-24T22:33:51Z

core/commands/cid.go

+			opts.newBase = mbase.Encoding(-1)
+		}
+
+		res, err := formatCids(req.Arguments, opts)


I'd stream these results back. Users may want to feed quite a few CIDs into this command.

I thought about that but didn't see the benefit. This is a client side utility that will never block and is should be very fast, so the only benefit will be memory usage. In addition the number of cid's passed in is limited by the length of command line so memory usage will never get out of control.

Ah. Well, actually, arguments can be passed in on stdin (newline separated)... That's why I brought this up. Basically, a user may run something like ipfs refs -u -r QmThing | ipfs cid format -f '%c' | sort -u.

I think you have to do something special to make that work. Because when I tried it I get Error: argument "cid" is required. I actually think this it is a bit of a misfeature to take the arguments from stdin without having to pass in a special flag, but that something to debate later.

Fwiw, I agree it's a misfeature to take arguments from stdin without a flag -- I think it's pretty widespread convention now to use a single dash "-" argument to mean "read stdin instead of file". (And that would make just as much sense for "read stdin instead of literal".)

Stebalien · 2018-08-24T22:42:30Z

@alanshaw this adds a new ipfs cid command. I'm fine with keeping this command "go specific" (it's really just a utility command) but thought you might have some input.

Basically, it makes it easy to inspect CIDs and convert between various formats (bases, versions, etc.).

Stebalien · 2018-08-27T20:31:41Z

core/commands/cid.go

+			emit(str, err)
+		}
+	}()
+	resp.Emit(out)


You can just call Emit on each CID individually (no need for the goroutine or the channel).

Stebalien · 2018-08-27T21:52:38Z

EnableStdin isn't broken, it's just that:

The documentation is all missing.
The API sucks.

Basically, you'll have to:

Iterate over all the arguments in Arguments.
Iterate over the arguments from BodyArgs().

You can also just call ParseBodyArgs() to move all the arguments from the body into Arguments.

kevina · 2018-08-27T22:14:04Z

You can also just call ParseBodyArgs() to move all the arguments from the body into Arguments.

That will prevent streaming.

Stebalien · 2018-08-30T02:17:24Z

That will prevent streaming.

It will. The important part here is allowing ourselves to stream at some point in the future without breaking the API.

kevina · 2018-08-30T02:19:34Z

It will. The important part here is allowing ourselves to stream at some point in the future without breaking the API.

Please see the new commits. You can now stream. Sorry I didn't follow up in the comments.

Stebalien

Is there any way to simplify this? We're introducing multiple new abstractions.

Stebalien · 2018-08-30T02:34:24Z

core/commands/cid.go

+		cmds.Text: cmds.MakeEncoder(func(req *cmds.Request, w io.Writer, val0 interface{}) error {
+			prefixes, _ := req.Options["prefix"].(bool)
+			numeric, _ := req.Options["numeric"].(bool)
+			val := val0.([]CodeAndName)


For now, you'll need to check if the type is correct. See: ipfs/go-ipfs-cmds#65. Basically, we end up feeding errors to this function as well.

(search the code for examples on how to do this).

Stebalien · 2018-08-30T02:35:50Z

core/commands/commands.go

+
+// streamRes is a helper function to stream results, that possibly
+// contain with non-fatal, the helper function is allowed to panic on
+// internal errors


Needs some proof-reading :).

Stebalien · 2018-08-30T02:37:54Z

core/commands/commands.go

+// streamRes is a helper function to stream results, that possibly
+// contain with non-fatal, the helper function is allowed to panic on
+// internal errors
+func streamRes(procVal func(interface{}, io.Writer) nonFatalError) func(*cmds.Request, cmds.ResponseEmitter) cmds.ResponseEmitter {


Can we call this streamResults? We tend to a bit too curt.

core/commands/commands.go

Stebalien · 2018-08-30T02:41:29Z

core/commands/cid.go

+		if !ok {
+			break
+		}
+		emit := func(fmtd string, err error) {


Let's lift this out of the loop (I don't trust go).

It's in the loop as it is a closure. I do it that way to avoid having to pass is the cidStr variable.

I just eliminated the need for the emit function.

Stebalien · 2018-08-30T02:41:40Z

core/commands/cid.go

+			if err != nil {
+				res.ErrorMsg = err.Error()
+			}
+			resp.Emit(res)


We should handle this error (the client may have gone away).

I'm not sure what I should do. Should I log the error and then just return?

I made at attempt at handling this, please check that it is correct.

kevina · 2018-08-30T03:01:11Z

Is there any way to simplify this? We're introducing multiple new abstractions.

Most (all) of the abstractions are lifted from the filestore ls code when I am doing nearly the same thing. Most of the completely comes from the fact that I believe fairly strongly that we should not abort on non-fatal errors, especially when streaming.

kevina · 2018-08-30T03:07:53Z

Is there any way to simplify this? We're introducing multiple new abstractions.

On second thought. I am not 100% sure what you mean. I thought you where talking about the handling of the cid fotmat command and streaming? Is there anything else you are referring to?

alanshaw · 2018-09-13T15:46:31Z

Hey everyone! I've tried to replicate the functionalty here as closely as possible for the jsipfs cid command. I'd appreciate it if you're able to spare a few minutes to look over https://github.com/ipfs-shipyard/js-cid-tool (if not the code, the README will do) to check I've captured the intentions correctly.

thank you ❤️

This PR adds a `jsipfs cid` command with tools for formatting, converting and discovering properties of CIDs. All the docs are here: https://github.com/ipfs-shipyard/js-cid-tool Will resolve this for JS: ipfs/kubo#5229 go-ipfs counterpart: ipfs/kubo#5385 License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

kevina · 2018-09-14T05:53:51Z

@alanshaw I looked over the README and I think the tool captures the intentions about the same. It was never really my intention for these commands to become standardized except for perhaps the base32 command.

Note that some minor details of the behavior of these commands may change over time.

kevina · 2018-09-14T06:12:56Z

@Stebalien if this is too complicated, I can separate out the part that implements proper streaming into a separate p.r. as I think that is where most of the complexity your seeing is coming from...

alanshaw · 2018-09-14T08:11:03Z

Thanks @kevina

It was never really my intention for these commands to become standardized

Understood, I haven't added this to any interface-* repos. I'm happy for this to just be a command line tool. I think it's important that jsipfs users also have it which is why I created it. Plus it was pretty quick and easy to put together.

kevina · 2018-09-20T07:26:06Z

@Stebalien I rebased this to fix the merge conflict and update to the new command libraries. It still seams that GitHub is broken when displaying the changes. Let me know if you made any progress with GitHub support or if you want me to try creating a new p.r.

Stebalien · 2018-09-25T05:26:35Z

LGTM but I'm pulling in @warpfork for a second signoff (especially on the API, we can always fix the code later).

Kubuxu

SGWM.

It would be really nice if we could differentiate between API and until tool but we can. So I guess this is API now.

Stebalien · 2018-10-01T17:49:21Z

@kevina I was just waiting on some second signoff. Now that we have one, this should be good to go. Unfortunately, it'll need a rebase. I'd squash first as doing this commit-by-commit is painful (I tried, but there were significant changes to the commands lib).

kevina · 2018-10-01T17:55:10Z

@Stebalien I rebase later today.

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Note reading input from Stdin is broken. Only the first result is accepted. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

This also avoid using TabWriter as it doesn't support right aligning a single column and also because its an overkill for this case. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina · 2018-10-01T18:18:33Z

@Stebalien actually I already rebased and fixed my code to use the new commands library as indicated in an earlier comment. I just rebased again and fixed a dep/import problem.

This should be good to go. There is one failed test in Jenkins, but I don't think that is related.

kevina · 2018-10-01T18:32:22Z

And it looks like rebasing caused GitHub to dismiss the earlier approving reviews...

kevina · 2018-10-01T19:12:34Z

@Stebalien Jenkins is now green.

Stebalien · 2018-10-02T22:54:42Z

And it looks like rebasing caused GitHub to dismiss the earlier approving reviews...

Yeah, I just turned that off. Sorry.

This PR adds a `jsipfs cid` command with tools for formatting, converting and discovering properties of CIDs. All the docs are here: https://github.com/ipfs-shipyard/js-cid-tool Will resolve this for JS: ipfs/kubo#5229 go-ipfs counterpart: ipfs/kubo#5385 License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

kevina added the topic/cidv1b32 Topic cidv1b32 label Aug 15, 2018

kevina requested a review from Kubuxu as a code owner August 15, 2018 23:15

ghost assigned kevina Aug 15, 2018

ghost added the status/in-progress In progress label Aug 15, 2018

kevina force-pushed the kevina/cid-cmd branch 4 times, most recently from 1dc7a95 to b46e3d2 Compare August 22, 2018 01:49

Stebalien reviewed Aug 24, 2018

View reviewed changes

Stebalien reviewed Aug 27, 2018

View reviewed changes

kevina force-pushed the kevina/cid-cmd branch from 113f8fc to 8d1b52d Compare August 28, 2018 00:36

Stebalien reviewed Aug 30, 2018

View reviewed changes

kevina force-pushed the kevina/cid-cmd branch from a30c487 to 11e3ed1 Compare August 30, 2018 07:37

This was referenced Sep 12, 2018

Sort outputs multiformats/js-cid-tool#1

Open

Implement format command multiformats/js-cid-tool#2

Closed

alanshaw mentioned this pull request Sep 13, 2018

feat: add cid command ipfs/js-ipfs#1560

Merged

kevina force-pushed the kevina/cid-cmd branch 2 times, most recently from 2973fdb to e21f234 Compare September 20, 2018 07:23

kevina mentioned this pull request Sep 21, 2018

Migration towards base32 CIDv1 #5358

Open

8 tasks

Stebalien previously approved these changes Sep 25, 2018

View reviewed changes

Stebalien requested a review from warpfork September 25, 2018 05:25

Kubuxu previously approved these changes Sep 26, 2018

View reviewed changes

kevina added 10 commits October 1, 2018 14:05

Provide new "cid" sub-command.

57f9601

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Remove "prefix" format string. Use WithDefault("%s') for `-f- option.

30b861b

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Factor out boiler plate code for PostRun in "ipfs filestore ls".

98e5999

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Stream results for "ipfs cid format".

e5622f6

Note reading input from Stdin is broken. Only the first result is accepted. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Right align numbers in "ipfs cid bases|codecs|hashes" output.

599bb73

This also avoid using TabWriter as it doesn't support right aligning a single column and also because its an overkill for this case. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Reenable Stdin argument reading.

f3ea21c

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Add basic sharness tests.

88a7e58

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Address code review.

c0a873c

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Eliminate emit closure, so something with errors on call to emit.

7edc7de

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Rename streamResults to streamResult.

54904a7

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina dismissed stale reviews from Kubuxu and Stebalien via 54904a7 October 1, 2018 18:09

kevina force-pushed the kevina/cid-cmd branch from 2973fdb to 54904a7 Compare October 1, 2018 18:09

Stebalien merged commit 021fc9f into master Oct 2, 2018

Stebalien deleted the kevina/cid-cmd branch October 2, 2018 22:55

ghost removed the status/in-progress In progress label Oct 2, 2018

Provide new "cid" sub-command. #5385

Provide new "cid" sub-command. #5385

Conversation

kevina commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 24, 2018

Choose a reason for hiding this comment

Stebalien commented Aug 27, 2018

kevina commented Aug 27, 2018

Stebalien commented Aug 30, 2018

kevina commented Aug 30, 2018

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina commented Aug 30, 2018

kevina commented Aug 30, 2018

alanshaw commented Sep 13, 2018

kevina commented Sep 14, 2018

kevina commented Sep 14, 2018

alanshaw commented Sep 14, 2018

kevina commented Sep 20, 2018

Stebalien commented Sep 25, 2018

Kubuxu left a comment

Choose a reason for hiding this comment

Stebalien commented Oct 1, 2018

kevina commented Oct 1, 2018

kevina commented Oct 1, 2018

kevina commented Oct 1, 2018

kevina commented Oct 1, 2018

Stebalien commented Oct 2, 2018

kevina commented Aug 15, 2018 •

edited

Loading