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.

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

Implement the -q or --quiet flag to add with minimal output #372

Merged
merged 2 commits into from
Dec 5, 2014

Conversation

chriscool
Copy link
Contributor

This implements issue #304.

It was done as much as possible in the same way as the
-r or --recursive flag.

License: MIT
Signed-off-by: Christian Couder chriscool@tuxfamily.org

@chriscool
Copy link
Contributor Author

Ooops, I had not checked that the go tests still passed. It should be ok now 😄

if quietOpt != nil && quietOpt.Definition() == cmds.OptionQuietOutput {
quiet, _, err = quietOpt.Bool()
if err != nil {
return nil, nil, nil, u.ErrCast()
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the Request instance was already successfully initialized, we need to still return it:

   return req, nil, nil, u.ErrCast()

(see #378)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right, I will fix that.

@jbenet
Copy link
Member

jbenet commented Nov 23, 2014

Great! Though-- at first pass I think this can be implemented without changing the NewRequest signature and without modifying the cli parsing, which are both already really complex... :( [1]

While the Recursive flag affects the request before it goes out, the quiet flag may be able to function like any other flag. I think the static option is all we need. So within a cmd, we would:

// within the cmd's Run func:
quiet, _, err := req.Option("quiet").Bool() // [2]
if err != nil {
    return nil, err
}

[1] I'd like to avoid adding more complexity into the request + parsing-- though if we determine it really should be this way, let's do it. I just don't see it yet.
[2] i think it's fine to explicitly ask for --quiet, and not support both -q and --quiet if it makes things easier on the implementation.

@chriscool
Copy link
Contributor Author

Ok, I will have another look at this.

@btc btc added the status/in-progress In progress label Nov 24, 2014
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool
Copy link
Contributor Author

@jbenet the new version uses the code you suggested and it accepts both --quiet and -q

@jbenet
Copy link
Member

jbenet commented Dec 5, 2014

@chriscool awesome, thanks! LGTM! (btw, i'm fine with adding static options (like a dedicated quiet option, that works the same way), just want to avoid adding complexity to the NewRequest, etc. we can always just add it the next time we add a quiet flag.

jbenet added a commit that referenced this pull request Dec 5, 2014
Implement the -q or --quiet flag to add with minimal output
@jbenet jbenet merged commit 11508f8 into ipfs:master Dec 5, 2014
@jbenet jbenet removed the status/in-progress In progress label Dec 5, 2014
@cryptix cryptix mentioned this pull request Mar 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants