Command Interface

[mappum]

This isn't ready for merge yet, PRing for code review/comments.

[jbenet]

Couple style things, setup your editor to automatically run go fmt, golint, and go build. There's nice packages for all editors out there.

[jbenet]

if we have a Request object, maybe could return a Response object too.

[jbenet]

looks good so far!

[mappum]

The recent changes deal with command output. Commands will now be able to output an error or any interface{}, and it can then be easily marshaled based on the command caller's choice of encoding type.

[jbenet]

can make this struct implement the error interface by adding:

func (e *Error) Error() string {
  return fmt.Sprintf("%d error: %s", e.Code, e.Message)
}

http://golang.org/pkg/builtin/#error

And then just use it directly in the Response struct

[jbenet]

bump on this. it simplifies the Response implementation

[jbenet]

addressed

[jbenet]

LGTM so far. Not sure why travis failed-- maybe try rebasing on master (not sure if up to date)

[jbenet]

@mappum lmk when you want CR / ready to merge in.

[mappum]

The commands package is essentially done. LMK if any parts of the API look like they need polishing.

Next up is refactoring the commands, CLI entry point, and RPC client/server to use this. That is a big change, so it could possibly be in its own PR (and this could now be merged).

[jbenet]

• f -> function (meaningful var name at object level)
• make a type for the function
• comments on exported symbols (golint)

// Function is the type of function that Commands use. 
// It reads from the Request, and writes results to the Response.
type Function func(*Request, *Response)

// Command is a runnable command, with input arguments and options (flags).
// It can also have subcommands, to group units of work into sets.
type Command struct {
    Help        string
    Options     []Option

    function    Function
    subcommands map[string]*Command
}

[jbenet]

addressed

[jbenet]

@mappum rebased on top of new master.

[jbenet]

@mappum: one comment re Parse returning Request. other changes LGTM.

one caveat i have on this is i think that the Register call won't let us define commands statically. It would be good to be able to continue defining things statically, and then have the options checked on dispatch (i.e. either in Run (but this may do work more than once) or when constructing the Request).

[jbenet]

actually, hang on. it may not be needed, given that it's cumbersome to define it statically, and we may want functions to help us with making options etc:

// in a function
cmdIpfsPin = &commands.Command{
  Description:     "pin ipfs objects to local storage.",
  Help: `Manages ipfs objects pinned to (kept in) local storage.`,
  Options: []Option{
    Option{[]string{"r", "recursive"}, commands.Bool},
  },
  Run: func(req Request, res Response) { ... },
  Subcommands: map[string]*Command{
    "add": cmdIpfsPinAdd,
    "rm": cmdIpfsPinRm,
  }
}

or

var cmdIpfsPin = &cmd.Command{
  Description:     "pin ipfs objects to local storage.",
  Help: `Manages ipfs objects pinned to (kept in) local storage.`,
  Options: []Option{
    cmd.MakeOption(cmd.Bool, "pin objects recursively", "r", "recursive")
  },
  Run: func(req Request, res Response) { ... },
}
cmdIpfsPin.Register("add", cmdIpfsPinAdd)
cmdIpfsPin.Register("rm", cmdIpfsPinRm)
// I really dont like this o/  it makes it easier to change commands after startup, or 
// to move this Register call elsewhere in the code. 

And, autogenerating the "subcommands" part of the help from the subcommands map won't work well: it won't let us specify order.

[jbenet]

@mappum: did you opt for keeping a separate Stream instead of assigning to the same value?

ok, let's merge this and see how the interface has to change when we fit the other commands to it.

[jbenet]

Awesome