Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/commands/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ remains to be implemented.
},

Arguments: []cmds.Argument{
cmds.FileArg("path", true, true, "The path to a file to be added to IPFS").EnableRecursive().EnableStdin(),
cmds.FileArg("path", false, true, "The path to a file to be added to IPFS. If not set, reads the file content from standard input.").EnableRecursive().EnableStdin(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
Options: []cmds.Option{
cmds.OptionRecursivePath, // a builtin option that allows recursive paths (-r, --recursive)
Expand Down