It's currently impossible to correctly handle server-side context timeouts.

• Option 1: Call SetError(ctx.Err()). This will panic.
• Option 2: Simply return and close the channel. The outer handler may not have seen the context cancellation yet so it may decide to proceed and close the connection as if everything worked out as expected.

This package absolutely MUST NOT depend on go-ipfs.

Currently, if I try to update cmdkit, I get failures because go-ipfs uses a different version of go-ipfs-cmdkit (and we leave go-ipfs gx-rewritten). However, I can't update go-ipfs-cmdkit in go-ipfs without updating it here first (because they both depend on go-ipfs-cmdkit).

I can just update this, break the tests, update go-ipfs, and then re-test but that's not something we should be doing.

There is a weird race condition that usually hits during the add sharness tests. If it happens, the last line of the output is missing. Needs investigation!

Having to constantly parse/manage options is a bit annoying. We could, instead, use the "option" values to do this for us.

// Option is used to specify a field that will be provided by a consumer
type Option interface {
	Name() string    // the main name of the option
	Names() []string // a list of unique names matched with user-provided flags

	// NOTE: I've removed `Kind()`. It doesn't need to be on the interface.
	Description() string // a short string that describes this option

	// NOTE: I've removed the `WithDefault` method. Each option type would provide its own typesafe version.
	// NOTE: I've removed `Default()`. We don't actually need it (`Get` below can create defaults as needed). However, there isn't much of a reason to *not* have it (other than complexity)...

	// NOTE: still want this to allow us to validate all options up-front. We could do that as-needed but I'd rather handle errors this way.
	Parse(str string) (interface{}, error)
}

type StringOption struct {
	Name        string
	Synonyms    []string
	Description string
	// anything else...
}

type StringOptionWithDefault struct {
	StringOption
	Default string
}

func (s *StringOption) Get(req *Request) (string, bool) {
	v, ok := req.Options[s]
	if !ok {
		return "", false
	}
	return v.(string)
}

func (s StringOption) WithDefault(s string) StringOptionWithDefault {
	StringOptionWithDefault{
		StringOption: s,
		Default:      s,
	}
}

func (s *StringOptionWithDefault) Get(req *Request) string {
	if v, ok := s.StringOption.Get(req); ok {
		return v
	}
	return s.Default
}

var MyOption = StringOption{
	Name:        "n",
	Synonyms:    []string{"testing"},
	Description: "description....",
}.WithDefault("foobar")

func MyCommand(req *Request) {
	myString := MyOption.Get(req)
}

@keks thoughts?

This should be a general-purpose library.
Main issue is the Context struct, which contains and ipfs node and config. My current idea is to replace it with an additional argument env of type interface{} that can be passed to cmd.Call() and will be passed to PreRun and PostRun.

Currently the caller has to call PreRun and PostRun themselves, and has to write different calling code, depending on whether the command should be run locally or remotely via http. I want

• To add an Executor interface that is implemented by e.g. "local".Executor and "http".Executor. Using the local executor will run PreRun, then Run and PostRun. Using the http executor will run PreRun, send a request to the server, then run PostRun to handle the response.
• go-ipfs' main.go to be much shorter, moving the details of what happens when a command is called to go-ipfs-cmds.

Both
-https://golang.org/pkg/flag/ and

• https://github.com/spf13/pflag

seem like good and battle tested options to parse commandline flags, why is this library reimplementing all of that?

1. make my commits commits -> related work currently spread over several commits at different points in history
2. rebase my commits in go-ipfs to HEAD so it can be easily merged

• #19 Put less burden on the caller

After 1.0 I want to start moving all commands to the new API. After that transition is done, we can remove go-ipfs/commands/...

Currently, we allow users to mix optional, variadic, and required arguments. In order of preference:

1. make this impossible (see below)
2. check at start
3. check when calling the command

We currently do none of these. Instead, we have some weird logic in CheckArguments where we stop filling in optional arguments once we realize we'll need the rest to fill in required arguments.

Preferred solution:

type Command struct {
	Arguments Arguments
	// ...
}
    
type Arguments struct {
	Required []Argument
	Optional []Argument
	Variadic Argument
}

This way, we can avoid the whole parsebodyargs thing and always use args lazily.

I think it provides a pretty poor UX to have Emit return errors. It complicates the calling code a bit (what do we even do with the errors? we cant use SetError). Lets discuss how to address this.

So, the old interface provided access to stderr. The new one assumes that all values emitted should go to stdout. It assumes that all errors should be returned via SetError. Unfortunately, this makes it impossible to migrate some commands without breaking the API.

Also, as I've stated in #62, I'd like to just use SetError for terminal errors. That is, "status" errors are just output.

We merged it to get the fixes into go-ipfs ASAP (and unblock other merges into go-ipfs).

See #38 (comment)

Currently, depending on circumstances arrays can be either Marshalled to JSON as null or []. It would be best if we send just one format, possibly the [].

I think I can make the legacy layer smaller. cmds.Command could have a OldCommand *oldcmds.Command that just points to the faked old Command. For commands that already use the new API this is just nil. That way I don't need to convert back and forth.

Need to check whether this really makes sense, though.

Somehow requests going to /api/v0/* don't end up in our ServeHTTP handler.

• #3 Make Request pretty

• #20 Remove go-ipfs deps

  • extract legacy pacakge
  • legacy: the user of cmds has to convert a "go-ipfs"/command.Command to a "go-ipfs-cmds".Command themselves using the legacy package
    • before cmds.Command had two maps for subcommands, one with cmds.Command values and one with commands.Command values
  • move legacy to go-ipts/commands/legacy
  • go-ipfs-cmds/http depends on go-ipfs/repo/config to get ipfs' version ce22f51
    • let the caller set a user agent
    • let the caller set a version string that needs to be the same on client and server

• cmdkit: rename Option.Default to WithDefault and Option.DefaultVal to Default

• cmdkit: when an option is passed, only use the first (i.e. long) flag name as key in the map, instead of every name

  • this one requires changes all over go-ipfs-cmds, go-ipfs-cmdkit, go-ipfs/commands, go-ipfs/core/commands
  • go-ipfs-cmdkit ipfs/go-ipfs-cmdkit@a03ffa7
  • go-ipfs-cmds df00b75
  • go-ipfs/commands
  • go-ipfs/core/commands

• drop globalOptions, instead add them as Options to the root command

• move predefined options from cmdkit to cmds

• make the legacy layer work again after all these changes

  • this one will be bad

Working branches:

• https://github.com/ipfs/go-ipfs-cmds/tree/cmds0.5
• https://github.com/ipfs/go-ipfs/tree/feat/cmds0.5
• https://github.com/ipfs/go-ipfs-cmdkit/tree/cmdkit1.0

There's a few issues with the http API and it would be cool to support different kinds of transports like Unix domain sockets or websockets. So having a protocol that runs atop any reliable transport would be desirable.

This issue is about exploring the problem space and having a place to put ideas that need to be considered when we finally start working on this.

Relevant issues:

• #31 Make sure we can distinguish error from value at all times
• ipfs/kubo#4218 Add support for Unix domain sockets

ipfs key gen's help text shows:

ipfs key gen [--type=<type> | -t] [--size=<size> | -s] [--] <name>

However, --type and --size are required options (command errors out if they are not supplied). If these are intended to be required, it should probably look like:

ipfs key gen (--type=<type> | -t) (--size=<size> | -s) [--] <name>

E.g., Cli { struct options }.Run(ctx). It's a nice hack to get named (optional) parameters and avoid functions with many parameters.

We need to make it clear whether the emitter is simply closed (and the sender should go away) or something actually went wrong.

Really, we should probably just log what went wrong and return a boolean where false means "closed". Currently, we return an error but it's unclear how this error should be handled. Should the caller follow up with a SetError call? (the answer is no but the API is really unclear).

Also, we need to make sure that all emitters handle context cancellation correctly.

If I use MakeTypedEncoder, with a concrete type, say *Foo then the error case is not handled correctly anymore, as SetError calls re.Emit but not with type *Foo but rather *cmdkit.Error.

This results in a panic of unexpected type: *cmdkit.Error

Handling of stdin arguments doesn't take optional arguments into account.

That is, either:

{
    "Type": "error",
    "Code": 0,
    "Message": "my message"
}

Or:

{
    "Type": "value",
    "Value": { /* ... */ }
}

Currently, one can break the API as follows:

> ipfs dag put <<<'{"Message": "the api is broken", "Code": 0, "Type": "error"}'
zdpuAxiFRrxtmMmFRZx145LufQsKTw8is1ZgfTQX4vgzgb8DB

> ipfs dag get zdpuAxiFRrxtmMmFRZx145LufQsKTw8is1ZgfTQX4vgzgb8DB
Error: the api is broken

https://github.com/ipfs/go-ipfs-cmds/blob/master/cli/helptext.go#L127

$ ipfs pin --help
USAGE
  ipfs pin - Pin (and unpin) objects to local storage.

SYNOPSIS
  ipfs pin

https://github.com/ipfs/go-ipfs/pull/4012/files

There are cases where I need to handle two errors at once. In those cases I use the variable names err and err_. It's just like on IRC, if a name is aready taken you use ${name}_, or in maths, e.g. i and i'.

I think this is a valid use of an underscore in a variable name, but I can't select WontFix in codeclimate since I'm not authorized.

@Kubuxu @whyrusleeping what do you think?

Currently the tests don't run because we depend on go-ipfs and that depends on a lot of gx stuff.

Depends on #20.

Note: To run the tests you have to gxify the whole thing:

gx-go rewrite
go test ./...
gx-go rewrite --undo

When running ipfs refs bad-path, I'll ocationally see the following panic:

18:48:04.074 ERROR  cmds/http: a panic has occurred in the commands handler! handler.go:125
18:48:04.074 ERROR  cmds/http: connection already closed / custom - http.respem - TODO handler.go:126
18:48:04.074 ERROR  cmds/http: stack trace:
goroutine 351 [running]:
runtime/debug.Stack(0xc42015f280, 0xc42067a1f0, 0x1)
	/usr/lib/go/src/runtime/debug/stack.go:24 +0xa7
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http.internalHandler.ServeHTTP.func1()
	/home/steb/projects/go/src/gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http/handler.go:127 +0x130
panic(0x12d0680, 0xc42067a090)
	/usr/lib/go/src/runtime/panic.go:491 +0x283
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http.(*responseEmitter).SetError(0xc420278e10, 0x12d0680, 0xc42004b960, 0x0)
	/home/steb/projects/go/src/gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http/responseemitter.go:159 +0x228
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds.(*fakeResponse).SetError(0xc4201e3040, 0x1cb8260, 0xc42004b960, 0x0)
	/home/steb/projects/go/src/gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/legacy.go:190 +0xa6
github.com/ipfs/go-ipfs/core/commands.glob..func114(0x1cce6a0, 0xc42065ff00, 0x1ccd700, 0xc4201e3040)
	/home/steb/projects/go/src/github.com/ipfs/go-ipfs/core/commands/refs.go:112 +0x3be
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds.NewCommand.func1(0x1cce5e0, 0xc420326480, 0x6bc35a6cbf60, 0xc420278e10)
	/home/steb/projects/go/src/gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/legacy.go:404 +0x197
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds.(*Command).Call(0x1d9c0a0, 0x1cce5e0, 0xc420326480, 0x6bc35a6cbf60, 0xc420278e10, 0x0, 0x0)
	/home/steb/projects/go/src/gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/command.go:123 +0x1de
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http.internalHandler.ServeHTTP(0x0, 0xc42001c08a, 0x67, 0xc4205386f0, 0xc4201b6480, 0x15770f0, 0xc4201a8700, 0xc4203544f0, 0x1d9c0a0, 0xc420538690, ...)
	/home/steb/projects/go/src/gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http/handler.go:194 +0xa0d
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http.(*internalHandler).ServeHTTP(0xc4201e4aa0, 0x1cc2ae0, 0xc42000e720, 0xc420345e00)
	<autogenerated>:1 +0x72
gx/ipfs/QmPG2kW5t27LuHgHnvhUwbHCNHAt2eUcb4gPHqofrESUdB/cors.(*Cors).Handler.func1(0x1cc2ae0, 0xc42000e720, 0xc420345e00)
	/home/steb/projects/go/src/gx/ipfs/QmPG2kW5t27LuHgHnvhUwbHCNHAt2eUcb4gPHqofrESUdB/cors/cors.go:188 +0x108
net/http.HandlerFunc.ServeHTTP(0xc420528420, 0x1cc2ae0, 0xc42000e720, 0xc420345e00)
	/usr/lib/go/src/net/http/server.go:1918 +0x44
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http.Handler.ServeHTTP(0x0, 0xc42001c08a, 0x67, 0xc4205386f0, 0xc4201b6480, 0x15770f0, 0x0, 0xc4203544f0, 0x1d9c0a0, 0xc420538690, ...)
	/home/steb/projects/go/src/gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http/handler.go:117 +0x5e
gx/ipfs/QmP9vZfc5WSjfGTXmwX2EcicMFzmZ6fXn7HTdKYat6ccmH/go-ipfs-cmds/http.(*Handler).ServeHTTP(0xc42050a540, 0x1cc2ae0, 0xc42000e720, 0xc420345e00)
	<autogenerated>:1 +0x78
net/http.(*ServeMux).ServeHTTP(0xc42035dce0, 0x1cc2ae0, 0xc42000e720, 0xc420345e00)
	/usr/lib/go/src/net/http/server.go:2254 +0x130
net/http.(Handler).ServeHTTP-fm(0x1cc2ae0, 0xc42000e720, 0xc420345e00)
	/usr/lib/go/src/net/http/h2_bundle.go:5462 +0x4d
gx/ipfs/QmX3QZ5jHEPidwUrymXV1iSCSUhdGxj15sm2gP4jKMef7B/client_golang/prometheus.InstrumentHandlerFuncWithOpts.func1(0x1cc4160, 0xc42074c8c0, 0xc420345e00)
	/home/steb/projects/go/src/gx/ipfs/QmX3QZ5jHEPidwUrymXV1iSCSUhdGxj15sm2gP4jKMef7B/client_golang/prometheus/http.go:287 +0x26f
net/http.HandlerFunc.ServeHTTP(0xc4201e4a50, 0x1cc4160, 0xc42074c8c0, 0xc420345e00)
	/usr/lib/go/src/net/http/server.go:1918 +0x44
net/http.(*ServeMux).ServeHTTP(0xc42035dcb0, 0x1cc4160, 0xc42074c8c0, 0xc420345e00)
	/usr/lib/go/src/net/http/server.go:2254 +0x130
net/http.serverHandler.ServeHTTP(0xc42032ea90, 0x1cc4160, 0xc42074c8c0, 0xc420345e00)
	/usr/lib/go/src/net/http/server.go:2619 +0xb4
net/http.(*conn).serve(0xc42019b040, 0x1cc4fe0, 0xc4206901c0)
	/usr/lib/go/src/net/http/server.go:1801 +0x71d
created by net/http.(*Server).Serve
	/usr/lib/go/src/net/http/server.go:2720 +0x288
 handler.go:127

While I usually can't reproduce this easily, it reproduces readily when running the ./t0110-gateway.sh on commit cccd03e875ea961e96b225a910ae1f55d0eb83be (look at the trash directory.t0110-gateway.sh/daemon_err file).

PostRun is called only once in cmds/ipfs/main.go. It is very specific to cli output. That's why I don't think it should be in the command library. I haven't thought about a better place yet, though.

Add more examples. Specifically, showcase Marshaler and PostRun.

For libp2p projects that don't have a Makefile I have been using this minimal one:

gx:
	go get github.com/whyrusleeping/gx
	go get github.com/whyrusleeping/gx-go

deps: gx
	gx --verbose install --global
	gx-go rewrite

publish:
	gx-go rewrite --undo

I have also converted them to use this standard test script: https://github.com/ipfs/ci-helpers/blob/master/travis-ci/run-standard-tests.sh

Should I create a p.r. for this?

• handle failing type assertions
• handle errors returned by Emit
• SetError drops error returned by Emit -> add error return value
• handle errors returned by SetError

In the getContext call in SetRootContext (in request.go), we construct a context with a timeout and/or cancel but throw away the cancel functions. We need to actually cancel the context with a timeout.

This may not be a problem in practice (if the parent context is properly canceled, I don't believe it will be) but we should still fix it.

I am trying to implement a command which has Type either *big.Int or big.Cid.

1. When using no Type and only MakeEncoder and send big.NewInt(0) I end up with the error unexpected type float64 (expected interface {})
2. When I try to wrap the result in something like this:

type result struct {
  myint *big.Int
  cid *cid.Cid
}

and use Type: &result{}, I end up with result being nil when sending result{myint: big.NewInt(0)}

currently around 25%...

Sometimes commands have different outputs depending on the flags, but I would still like to get type safety using MakeTypedEncoder. So I had the idea to allow for the following interface

var myCmd = &cmds.Command{
  // ... 
  Types: []interface{type1, type2},
  Encoders: cmds.EncoderMap{
    cmds.Text: []Encoder{
      MakeTypedEncoder(func(req *cmds.Request, w io.Writer, t1 type1) error {
        // encode t1
      }),
      MakeTypedEncoder(func(req *cmds.Request, w io.Writer, t2 type2) error {
        // encode t2
      }),
    }
}

Where the library would go through and find the correct encoder given a certain type.

In addition I am seeing instances where I would really like to have default encoders registered for certain types, or for certain interfaces. So with the above a nice addition would be to be able to say register these encoders in a global fashion on cmds and then they are used as fallback if no matching typed encoder is found.

cmds.RegisterTypedEncoder(MakeTypedEncoder(func(req *cmds.Request, w io.Writer, t1 type1) error {
  // encode t1
}))
cmds.RegisterTypedEncoder(MakeTypedEncoder(func(req *cmds.Request, w io.Writer, t2 type2) error {
  // encode t2
}))

var myCmd = &cmds.Command{
  // ... 
  Types: []interface{type1, type2},
  // Encoders is not here, but it still works, because the encoders for type 1 and type 2 have been registered before
}

1. Remove support for HEAD.
2. Correct support for OPTIONS.
3. Deprecate support for GET.
4. Allow POST.
5. Return an error on all other methods.

Reproducible by running t0110-gateway.sh. Test 31 fails ("refs IPFS directory file through readonly API succeeds").

See ipfs/kubo#4453

Cause: Command fails and then we wait for it to succeed.

Especially the setters should go. A Request is created by someone and then passed on. They can make a struct that implement that interface, that is specific to the situation.

Also, maybe Request can even be a struct, like in net/http? I'm not sure that's a good idea, just thinking out loud...

We should not cancel the request context when we receive the last item. This breaks things. Unfortunately, we can't even cancel the context after Execute is done, for some reason. That still breaks things.

We could do this in Run but that doesn't feel like the right place to handle it.