Add command does not support json encoding

[travisperson]

Currently the add command does not support JSON encoding.

This is due to setting the output to nil.

A fix to this is to detect the output format and skip this PostRun if the encoding is anything other than Text.

v, found, err := req.Option(cmds.EncShort).String()
if err == nil && found && v != cmds.Text {
    log.Debugf("Encoding is not text, abort and let the marshaller handle it\n")
    return
}

This however introduced another issue. All output is returned as JSON. If the previoususerProvidedEncoding is not set, we keep the JSON encoding.

The reason we don't have a default encoding is due to using two different option keys, in the client.go file we use cmds.EncShort, while in the cli we use encoding.

However, there is another issue, since the add command does not use a marshaler, the check on line 240 will fail and switches to JSON.

PostRun is used for progress, and at some point the Marshaler was removed.

I don't know the history of this file, so there might of been a good reason (not used?). Anyways without a Marshaler the original encoding will never get set back.

This is a start of a PR I want to open

diff --git a/cmd/ipfs/main.go b/cmd/ipfs/main.go
index 2ad90cf..411b833 100644
--- a/cmd/ipfs/main.go
+++ b/cmd/ipfs/main.go
@@ -236,11 +236,11 @@ func (i *cmdInvocation) Parse(ctx context.Context, args []string) error {

    // if no encoding was specified by user, default to plaintext encoding
    // (if command doesn't support plaintext, use JSON instead)
-   if !i.req.Option("encoding").Found() {
+   if !i.req.Option(cmds.EncShort).Found() {
        if i.req.Command().Marshalers != nil && i.req.Command().Marshalers[cmds.Text] != nil {
-           i.req.SetOption("encoding", cmds.Text)
+           i.req.SetOption(cmds.EncShort, cmds.Text)
        } else {
-           i.req.SetOption("encoding", cmds.JSON)
+           i.req.SetOption(cmds.EncShort, cmds.JSON)
        }
    }

diff --git a/core/commands/add.go b/core/commands/add.go
index 2b3297e..b2fc45f 100644
--- a/core/commands/add.go
+++ b/core/commands/add.go
@@ -127,7 +127,18 @@ remains to be implemented.
            }
        }()
    },
+   Marshalers: cmds.MarshalerMap{
+       cmds.Text: func(res cmds.Response) (io.Reader, error) {
+           return nil, nil
+       },
+   },
    PostRun: func(req cmds.Request, res cmds.Response) {
+       v, found, err := req.Option(cmds.EncShort).String()
+       if err == nil && found && v != cmds.Text {
+           log.Debugf("Encoding is not text, abort and let the marshaller handle it\n")
+           return
+       }
+
        if res.Error() != nil {
            return
        }

I just want to get some feedback before making any commits.

/cc @jbenet @whyrusleeping @kyledrake

[travisperson]

There is another issue with this that the add command does not support xml

Error: xml: unsupported type: <-chan interface {}

I have yet to tract down where the exact issue is though.

[travisperson]

Also pretty sure moving encoding to cmds.EncShort, may not be the exact solution. I believe other places send encoding, though we can probably just clean it up.

[whyrusleeping]

i wonder if we can remove the postrun thing entirely and get away with just getting fancy with the marshaler... I do a lot of hacky shit (tm) with the marshaler in core/commands/dht.go

[travisperson]

Ping

[jbenet]

what's next here?

[travisperson]

Basically we need to figure out what is the best approach to solve this issue. I played around with the idea @whyrusleeping mentioned but ran into concurrency issues and printing to stdout. I unfortunately accidently did a git reset and lost my changes, but the idea was to use a marshaller channel.

This is the bit we talked about a while go: https://botbot.me/freenode/ipfs/2015-04-28/?msg=37740801&page=3

I'm not certain what the best approach is besides to not use the marshaller and simple correct (by applying the patch above or something similar) the issue of not resetting the encoding type.

[whyrusleeping]

heres what we can do, we can keep track of progress on the daemon side (within the Run function) and pass all the keys back as objects over the channel marshaler in an object that looks something like:

type AddUpdate struct {
    Key string
    Progress float64
}

[chriscool]

@travisperson you had not committed your changes ?
Anyway most of the time using git reset --keep is the safest option to use when you want to reset. It will not remove uncommited changes in the working directory.

[travisperson]

Doesn't really matter too much. They were easy changes and I can rewrite
them if it's really the way we want to go. However, I'm almost certain
it's not what we wanted to end up doing.

On Thu, May 21, 2015, 2:21 AM Christian Couder [email protected]
wrote:

@travisperson https://github.com/travisperson you had not committed
your changes ?
Anyway most of the time using git reset --keep is the safest option to
use when you want to reset. It will not remove uncommited changes in the
working directory.

—
Reply to this email directly or view it on GitHub
#1121 (comment).

[whyrusleeping]

has this been fixed? @travisperson

[chriscool]

For this to move forward, it would be a good idea to write sharness tests even if some of them fail.
When some tests are not working it is always possible to use "test_expect_failure" instead of "test_expect_success" and to commit those tests anyway. It is a good way to document what is not working but should work.

[jbenet]

Agree with Chris wholeheartedly
On Sat, Jan 2, 2016 at 02:41 Christian Couder [email protected]
wrote:

For this to move forward, it would be a good idea to write sharness tests
even if some of them fail.
When some tests are not working it is always possible to use
"test_expect_failure" instead of "test_expect_success" and to commit those
tests anyway. It is a good way to document what is not working but should
work.

—
Reply to this email directly or view it on GitHub
#1121 (comment).

[hsanjuan]

This is not fixed yet. Has the revamp of the commands lib made the fix easier maybe ?

[Stebalien]

No. See: ipfs/go-ipfs-cmds#115.

[bqv]

This has been idle for two years, what's going on here?

[hsanjuan]

Hi @bqv . What's going on is that this is low priority because it is cosmetic and there are some workarounds, while the fix is not super simple and has been blocked on a number of refactors.

But I agree that the case of /add is the most annoying because curl-ing the API to add directories recursively is not feasible.

Also related #7050 .

[bqv]

Gotcha, ok