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

Add a --no-pin option to ipfs add #1931

Merged
merged 1 commit into from
Nov 16, 2015
Merged

Conversation

eminence
Copy link
Contributor

@eminence eminence commented Nov 2, 2015

Implements a solution for #1908

This PR replaces #1909

@ghost
Copy link

ghost commented Nov 2, 2015

I still think this should maybe be called --pin instead, defaulting to true.

@eminence
Copy link
Contributor Author

eminence commented Nov 2, 2015

Totally reasonable. I'll try to defend the code as implemented by pointing out that seems to be precedent in ipfs for options that, when present, turn off some functionality:

  • We have ipfs add --only-hash instead of ipfs add --write=false

(Ok, I admit that's the only example I could find). I don't think I care either way, except that --no-pin seems to be easier to type than --pin=false. I am happy to change this if someone else votes in favor of @lgierth

@jbenet
Copy link
Member

jbenet commented Nov 2, 2015

i think in this case, we do want --pin=false.

@eminence
Copy link
Contributor Author

eminence commented Nov 2, 2015

Ok! But one small problem -- In the dev0.4.0 branch, there is no support for having bool options take true/false parameters. For example:

 $ ./ipfs add --pin=false
Error: Option 'pin' takes no arguments, but was passed 'true'

This was added in 107409c, but this commit is not part of the dev0.4.0 branch. Are there any plans to rebase this branch on master?

@rht
Copy link
Contributor

rht commented Nov 9, 2015

+1 dev0.4.0 rebase @whyrusleeping (this can be done with less risk than other PRs)

@jbenet
Copy link
Member

jbenet commented Nov 10, 2015

@eminence @whyrusleeping is rebasing dev0.4.0 shortly

Implements a solution for ipfs#1908

This PR replaces ipfs#1909

License: MIT
Signed-off-by: Andrew Chin <achin@eminence32.net>
@eminence
Copy link
Contributor Author

Ok, I force pushed a commit, due to the dev0.4.0 rebase. Please take another look. Thanks!

@whyrusleeping
Copy link
Member

This LGTM

@rht rht added the RFCR label Nov 16, 2015

if !pin_found { // default
dopin = true
}
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to set the default on the commands lib itself? (am not sure.)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is the js trick !!dopin, don't know if it works in golang.

@jbenet
Copy link
Member

jbenet commented Nov 16, 2015

This LGTM. i'll merge. we can always improve #1931 (comment) later

jbenet added a commit that referenced this pull request Nov 16, 2015
Add a --no-pin option to `ipfs add`
@jbenet jbenet merged commit 09d34db into ipfs:dev0.4.0 Nov 16, 2015
@jbenet jbenet removed the backlog label Nov 16, 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.

None yet

4 participants