core/commands/add: Command-line <path> argument are optional

[wking]

Adding a file from stdin works now, so pass that information along to
the user.

[whyrusleeping]

I dont think required should be set to false. The requirement will be satisfied by piping through stdin.

[jbenet]

i think required still needs to be true. there's a complication here with the cmds lib because stdin counts as satisfying the requirement.

this comes from viewing stdin as just another file. which is what many cli tools do, but may not be the best model.

[jbenet]

also, the text should ideally fit in one line because it will be munged and placed in various places. (we could perhaps come up with a char limit) maybe:

The path to a file to be added to IPFS. If not set, uses stdin.

? not sure if stdin is too unfriendly.

[wking]

On Mon, Apr 27, 2015 at 04:29:20PM -0700, Juan Batiz-Benet wrote:

•   cmds.FileArg("path", true, true, "The path to a file to be added to IPFS").EnableRecursive().EnableStdin(),
  

i think required still needs to be true. there's a complication here
with the cmds lib because stdin counts as satisfying the
requirement.

Hmm. I think we need to make a distinction there, and maybe
automatically add the wrapping braces (‘[]’) if EnableStdin() is
set (and auto-add a trailing ellipsis (‘[ …]’) if
EnableRecursive() is set.

this comes from viewing stdin as just another file. which is what
many cli tools do, but may not be the best model.

I like being able to add a file from stdin, but maybe don't bind the
two ideas (file paths from argument names or file content from stdin)
together so tightly. Paths strings and an input stream aren't
interchangeable (e.g. we can read owner/group IDs from an input path,
but we lose that information if we pipe the file content into stdin).

So should I be PRing commands/argument.go instead?

[jbenet]

I think we need to make a distinction there, and maybe automatically add the wrapping braces (‘[]’) if EnableStdin() is set (and auto-add a trailing ellipsis (‘[ …]’) if EnableRecursive() is set.

Yeah. though not sure whether it's worth the time yet? i doubt it will be a trivial change. shrug

Paths strings and an input stream aren't interchangeable (e.g. we can read owner/group IDs from an input path, but we lose that information if we pipe the file content into stdin).

Yeah, fair point.

So should I be PRing commands/argument.go instead?

maybe? it's a bit of a mess in there. if you want to learn it go for it. else-- what is the problem being solved here? what is wrong by keeping it as is for now? (the cmds lib will get a big facelift at some point, so we can make sure to address this then)

[wking]

On Mon, Apr 27, 2015 at 05:14:07PM -0700, Juan Batiz-Benet wrote:

… what is the problem being solved here? what is wrong by keeping it
as is for now? (the cmds lib will get a big facelift at some point,
so we can make sure to address this then)

I didn't realize ‘ipfs add’ could read from stdin until you told me it
could 1. It seemed easy enough to update the command so the docs
would reflect that reality, and why have fixable errors in your docs?

But I agree that this is more of a UX issue than a functional issue,
so if the cmds lib looks to hairy I'll just leave it alone ;).

[jbenet]

oh 👍 on the comment, for sure. fixing the stdin / file distinction seems more trouble than useful atm. sorry for being unclear!

[chriscool]

@wking and @jbenet I think that the documentation might need to change anyway if I add the --stdin option as discussed in issue #1141.

[whyrusleeping]

@wking update here? lets merge or close soon