Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Pin commands default to recursive #1823

Merged
merged 4 commits into from
Oct 16, 2015
Merged

Pin commands default to recursive #1823

merged 4 commits into from
Oct 16, 2015

Conversation

frrist
Copy link
Member

@frrist frrist commented Oct 10, 2015

Fix for #1801
All pinning commands are by default recursive unless with a flag specified.
Because of these changes, parsing pin commands needed to allow bool opts to be specified as true/false. (Had a little trouble following parse.go, but I think I got things right)

recursive direct
ipfs pin add ipfs pin add -r=false
ipfs pin add -r ipfs pin add -r=FALSE
ipfs pin add -r=true ipfs pin add --recursive=false
ipfs pin add --recursive=true

@jbenet jbenet added the backlog label Oct 10, 2015
whyrusleeping
whyrusleeping reviewed Oct 10, 2015
View reviewed changes
commands/cli/parse_test.go
test("-b", kvs{"b": ""}, words{})
test("-bs foo", kvs{"b": "", "s": "foo"}, words{})
test("-b", kvs{"b": true}, words{})
test("-bs foo", kvs{"b": true, "s": "foo"}, words{})
Copy link
Member

Choose a reason for hiding this comment

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

huh, i didnt know we could combine flags like this.

Copy link
Member

Choose a reason for hiding this comment

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

i know, right? @AtnNn made it great :)

jbenet
jbenet reviewed Oct 11, 2015
View reviewed changes
commands/cli/parse_test.go
test("-sb", kvs{"s": "b"}, words{})
test("-b foo", kvs{"b": ""}, words{"foo"})
test("--bool foo", kvs{"bool": ""}, words{"foo"})
test("-b foo", kvs{"b": true}, words{"foo"})
Copy link
Member

Choose a reason for hiding this comment

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

maybe also try:

test("-b true", kvs{"b": true}, words{"true"})
test("-b false", kvs{"b": true}, words{"false"})

(i.e. we don't let bools be separated by space, as we cannot distinguish -b <arg> from -b true, right?)

Copy link
Member

Choose a reason for hiding this comment

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

👍

@jbenet
Copy link
Member

jbenet commented Oct 11, 2015

@ForrestWeston great, thanks! having the bool stuff in will be useful for other bool options that currently didn't have this. 👍

@whyrusleeping just to check, happy with this UX (-r=false) or do you want -d?

@whyrusleeping
Copy link
Member

i'm okay with -r=false, your point about having both --recursive and --direct made sense,

@jbenet
Copy link
Member

jbenet commented Oct 12, 2015

@whyrusleeping we could break backwards compat, deprecate -r now, and move to -d, remove -r later. up to you.

@whyrusleeping
Copy link
Member

It makes more sense to think that 'recursive == false' implies direct than to try and figure out that 'direct == false' means recursive.

@jbenet
Copy link
Member

jbenet commented Oct 12, 2015

@whyrusleeping sounds good to me.

frrist added 3 commits October 13, 2015 17:11
@frrist
Added ability to specify true/false for bool opts
107409c 
License: MIT
Signed-off-by: ForrestWeston <Forrest.Weston@gmail.com>
@frrist
changed pinning to be recursive by default
0123914 
pin add, pin rm, and pin ls will be recursive unless
specified with '=false' eg. 'ipfs pin add -r=false <file>'
tests for pinning have been updated/added

License: MIT
Signed-off-by: ForrestWeston <Forrest.Weston@gmail.com>
@frrist
Added tests from code-review
a8fc65f 
License: MIT
Signed-off-by: ForrestWeston <Forrest.Weston@gmail.com>
@frrist
Copy link
Member Author

frrist commented Oct 14, 2015

@whyrusleeping @jbenet added tests as requested and rebased

@whyrusleeping
Copy link
Member

only test failure is fixed by: #1837

LGTM.

jbenet
jbenet reviewed Oct 14, 2015
View reviewed changes
core/commands/pin.go
ShortDescription: `
Removes the pin from the given object allowing it to be garbage
Recursively removes the pin from the given object allowing it to be garbage
Copy link
Member

Choose a reason for hiding this comment

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

the description + tagline should still say:

"Unpin an object...
"Removes the pin...

but can just add:

(by default, recursively. use -r=false for direct pins)

or something

Copy link
Member

Choose a reason for hiding this comment

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

(this is because is should not be able to turn --recursive=false on a command that "Recursively unpins an object from local storage"

@jbenet
Copy link
Member

jbenet commented Oct 14, 2015

Otherwise LGTM

@frrist
Updated Pin Remove Tagline and Description
9916f86 
License: MIT
Signed-off-by: ForrestWeston <Forrest.Weston@gmail.com>
@whyrusleeping
Copy link
Member

RFM?

jbenet added a commit that referenced this pull request Oct 16, 2015
@jbenet
Merge pull request #1823 from ForrestWeston/recurPin
d6297c7 
Pin commands default to recursive
@jbenet jbenet merged commit d6297c7 into ipfs:master Oct 16, 2015
@jbenet jbenet removed the backlog label Oct 16, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@jbenet jbenet

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frrist @jbenet @whyrusleeping