Commands Refactor Part 2

jbenet commented 9 years ago

We split #196 in two parts. this is the second. (#262 is the first). This builds on all the changes to commands/.

jbenet commented 9 years ago

warning

Rebasing this on top of master once #262 lands.

jbenet commented 9 years ago

okay, this should be ready. cc @mappum @maybebtc

btc commented 9 years ago

thanks for the git ninja work. very much appreciated.

will have a look

jbenet commented 9 years ago

This branch's diff --stat master

 Makefile                   |   5 ++
 cmd/ipfs2/.gitignore       |   2 +
 cmd/ipfs2/Makefile         |   7 ++
 cmd/ipfs2/README.md        |  29 +++++++++
 cmd/ipfs2/daemon.go        |  63 ++++++++++++++++++
 cmd/ipfs2/equinox.yaml     |   6 ++
 cmd/ipfs2/init.go          | 157 ++++++++++++++++++++++++++++++++++++++++++++
 cmd/ipfs2/ipfs.go          |  15 +++++
 cmd/ipfs2/main.go          | 234 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 core/commands2/add.go      |  90 ++++++++++++++++++++++++++
 core/commands2/cat.go      |  39 +++++++++++
 core/commands2/commands.go |  59 +++++++++++++++++
 core/commands2/log.go      |  30 +++++++++
 core/commands2/ls.go       |  78 ++++++++++++++++++++++
 core/commands2/publish.go  |  72 +++++++++++++++++++++
 core/commands2/root.go     | 127 ++++++++++++++++++++++++++++++++++++
 daemon2/daemon.go          |  25 +++++++
 server/http/http.go        |   2 +
 18 files changed, 1040 insertions(+)

jbenet commented 9 years ago

btc commented 9 years ago

@jbenet @mappum question regarding ipfs versus ipfs2. ipfs prints help text when no arguments provided. This is kinda helpful. Do we want the same from the new cli? (Presently, shows no message. see image below)

From here forward, will mark my comments with TODO checklists to help track state.

[x] :tiger:

jbenet commented 9 years ago

Yes, we definitely want help text printed out.

btc commented 9 years ago

@mappum

These argument hints are quite helpful.

ipfs

    cmdIpfsInit.Flag.Int("b", 4096, "number of bits for keypair")
    cmdIpfsInit.Flag.String("p", "", "passphrase for encrypting keys")
    cmdIpfsInit.Flag.Bool("f", false, "force overwrite of existing config")
    cmdIpfsInit.Flag.String("d", "", "Change default datastore location")

ipfs2

        cmds.Option{[]string{"bits", "b"}, cmds.Int},
        cmds.Option{[]string{"passphrase", "p"}, cmds.String},
        cmds.Option{[]string{"force", "f"}, cmds.Bool},
        cmds.Option{[]string{"datastore", "d"}, cmds.String},

[ ] :tiger: displaying argument hints in UX
[ ] :tiger: providing argument hint in commands.Option

jbenet commented 9 years ago

@mappum @maybebtc how's it going over here?

mappum commented 9 years ago

Other than generating the proper help text, a lot of the commands are done:

[x] add
[x] cat
[x] commands
[x] daemon
[x] diag
[x] init
[x] log
[x] ls
[x] name
- [x] publish
- [x] resolve
[x] pin
[x] unpin
[x] version
[ ] block
[ ] bootstrap
[ ] config
[ ] mount
[ ] objects
[ ] tour
[ ] update

jbenet commented 9 years ago

Other than generating the proper help text, a lot of the commands are done:

Awesome! I'll CR in a bit.

mappum commented 9 years ago

Progress:

[x] block
[x] bootstrap
[x] config
[x] mount
[ ] object
[x] tour
[x] update

Tomorrow I'll iterate though the implemented commands and make sure @maybebtc's suggestions are followed. Then the last step will be nice help text + user-facing error messages, and some CR, then this is ready for primetime!

jbenet commented 9 years ago

These commits are exciting! Should I start CR or wait @mappum @maybebtc ?

I feel like we're at about minute 8:30 of this video: https://www.youtube.com/watch?v=-lpxZEni6hs

btc commented 9 years ago

First, allow me to sweep through, ensure help messages exist for all commands, and de-noise a bit, so the CR can focus on interesting stuff.

btc commented 9 years ago

comparing old pin vs new pin

@whyrusleeping @jbenet @mappum

was ipfs pin add/rm. I'm looking at the new file. It reads ipfs pin/unpin. Which is preferred?

jbenet commented 9 years ago

ipfs pin add/rm

WWGD

— Sent from Mailbox

On Fri, Nov 7, 2014 at 6:58 PM, Brian Tiger Chow notifications@github.com wrote:

comparing old pin vs new pin @whyrusleeping @jbenet @mappum

was ipfs pin add/rm. I'm looking at the new file. It reads ipfs pin/unpin. Which is preferred?

Reply to this email directly or view it on GitHub: https://github.com/jbenet/go-ipfs/pull/263#issuecomment-62242970

mappum commented 9 years ago

@maybebtc I'm currently working on generating help text (the options/arguments parts, and formatting), so don't do more than just the descriptions.

I didn't realize pin was add/rm before, that is a better choice.

btc commented 9 years ago

@mappum you may want to pull the latest and check for conflicts. i added help text to most commands

btc commented 9 years ago

@jbenet @mappum

For the documentation of many commands, the argument is referred to as ipfs-path

ipfs pin add <ipfs-path> -

The argument is named object in ipfs2.

/s/object/ipfs-path?

btc commented 9 years ago

all commands have help text

btc commented 9 years ago

@mappum I split name into -> name, publish, resolve

@jbenet ready for CR once @mappum gives the OK.

jbenet commented 9 years ago

Yes. <ipfs-path> means a string in the ipfs format. Object is ambiguous

jbenet commented 9 years ago

@maybebtc name publish and name resolve should not be their own commands. ipfs publish is something different that happens to call ipfs name publish. Also ipfs name should be able to be repackaged as ipns easily

btc commented 9 years ago

@jbenet

The commands are

ipfs name publish

ipfs name resolve

Isn't this the desired configuration?

(I split the files, as they were in ipfs1)

jbenet commented 9 years ago

Oh, yes that's the configuration I meant (ipfs name publish and ipfs name resolve). Sorry I can't look at code easily on my phone and got the opposite from the comment

— Sent from Mailbox

On Fri, Nov 7, 2014 at 7:17 PM, Brian Tiger Chow notifications@github.com wrote:

The commands are ipfs name publish ipfs name resolve

Isn't this the desired configuration?

Reply to this email directly or view it on GitHub: https://github.com/jbenet/go-ipfs/pull/263#issuecomment-62243550

btc commented 9 years ago

@mappum don't rebase yet. ok? let's do that at the very end

mappum commented 9 years ago

I added the help text stuff (gave everything a Description field, and the made generated help text for subcommands).

I plan on cleaning up cmd/ipfs2/main.go a bit, and the object command still isn't ported, but that's all that is left. Fell free to do CR, and make sure help text/formatting is satisfactory.

jbenet commented 9 years ago

Okay cool, will CR tonight. @maybebtc confirm?

jbenet commented 9 years ago

Many individual comments, but major things:

[x] command func signature change (done in b17e40e55be5c8ab89b09c56fe3a02c3fb5812af, 9e7f32f25aa21c60733bebffe1588b2166453803)
[x] using Option + Arg helpers to construct options + args (Options: 0d40781a8154d263cfcbb7ad5818f4d821ad596c, 5ab57a54367df0697b997650dc67d06ecc860a57, Arguments: 5222ec671ee1e6dc9524fa284e9b4fe004205b4, db910805d17e527dc4bd530018a018a73eaadde)
[x] using Option helpers to return types values (done in 37e7d1a96f5032456386b7617e67a79f9cc4773c)
[x] HelpGenOptions for auto generated help
[x] only use exit in main, return back to main everywhere else (fixed as of 100f1ee077d5efbbfa7a751449d610a4a26d91ee)
[ ] ExecContext thing
[x] Help text output: use templates. building up the string bits by bits is not good.
[ ] Restore all output formats (e.g. add). don't change these!! (c3e2c13158cc1a62bc28bb202b3f29986db75bf6, 748baed993277955a74e3a821c84225cfaad44cc)
[x] Restore all logging (where did it go?) (4743cf3b59a3703c1eedfcfb8cc8b7ad36111d60)
[ ] Restore exact command logic (add changes command logic!!)
[x] Restore exact command help text -- it's changed. Improvements should be separate PR (done in 42a9e3358d80a1452f6b500822245444f672f9f6)

I'm halting here (at core/commands2/block.go, halfway through the PR) because this is a lot that will change lots of code. I don't want to waste time going through and pointing out that commands have changed any of {help text, logic, logging, output formats}.

Please be sure to comment on every thread with where it was addressed, etc, like last time

@mappum this will change a ton of code. Getting CR earlier is useful. Lots of these changes have to be rolled back across the codebase... please communicate with others-- it's wasting precious time.

jbenet commented 9 years ago

Anyway, the HTTP API stuff is great, and exciting! Can't wait to have it running and be able to use a browser to issue commands + get output!!! :D

btc commented 9 years ago

@mappum Unless a desire to change the ipfs --help output was expressed explicitly, the help text should match master

before after

mappum commented 9 years ago

@maybebtc Hmm, I'm not exactly sure where that differently formatted output is coming from in master, since this is what's in the code: https://github.com/jbenet/go-ipfs/blob/master/cmd/ipfs/ipfs.go#L26-L55

btc commented 9 years ago

I see. If it's auto-generated, then let's set it aside for now and focus on the low hanging fruit. Of the items on this list, what's been addressed so far?

https://github.com/jbenet/go-ipfs/pull/263#issuecomment-62301758

mappum commented 9 years ago

Some comments on some of the listed tasks:

[ ] HelpGenOptions for auto generated help

Instead of an option struct with booleans that decide which sections to generate, we could have string fields that override the sections (for example the root command would override the subcommands section with its categorized subcommand list). Thoughts?

[ ] Restore exact command logic (add changes command logic!!)

Add is the only command that changed its logic, and the reason is because we currently can't support transfer of multiple files over HTTP. (But I can implement multipart bodies that allow multiple files, but it will take a few hours of work first).

[ ] Restore all output formats (e.g. add). don't change these!!

At the moment we don't have a way to get the original file paths (which got printed in the original output of add), but we can send them in request headers. This can be implemented alongside multipart files.

jbenet commented 9 years ago

Instead of an option struct with booleans that decide which sections to generate, we could have string fields that override the sections (for example the root command would override the subcommands section with its categorized subcommand list).

:+1: i like this

Add is the only command that changed its logic, and the reason is because we currently can't support transfer of multiple files over HTTP. (But I can implement multipart bodies that allow multiple files, but it will take a few hours of work first).

Ah, okay! Add was one of the first commands I saw. Let's merge this in and worry about multipart later

At the moment we don't have a way to get the original file paths (which got printed in the original output of add), but we can send them in request headers. This can be implemented alongside multipart files.

We may not need to send the file paths to the daemon, the client tool could translate the output. What's the analog for HTTP?

FWIW, the real original way was relative paths. it got switched to absolute when daemonized (because the output was in the daemon). I think the tool should still print relative paths (or absolute paths if that's what it's fed). "It should do what makes sense given the context"

mappum commented 9 years ago

We may not need to send the file paths to the daemon, the client tool could translate the output. What's the analog for HTTP?

I looked for an HTTP way of doing it, but there isn't one. Though we can just use custom request headers (something like 'X-File-Path') and the daemon can get the path from that (and just leave the paths as empty strings if the header isn't there).

A quick optimization we could do is detect if the daemon is on the same machine as the client (as I suspect it will be a majority of the time), and just send the path strings in that case (instead of streaming all the file data). Then we can use multiple files, and get the path strings, without the multipart stuff (except it will only work if the daemon is on the local machine). And I'm sure it would be more performant, too.

jbenet commented 9 years ago

Maybe it's Content-Disposition?

mappum commented 9 years ago

Maybe it's Content-Disposition?

That's for filenames, so we will use that too, but not for full paths ('foo.txt' vs '~/textfiles/foo.txt').

jbenet commented 9 years ago

Closed in favor of #332

ipfs / kubo