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

Pin command fixes #2477

Merged
merged 2 commits into from
Mar 31, 2016
Merged

Pin command fixes #2477

merged 2 commits into from
Mar 31, 2016

Conversation

Stebalien
Copy link
Member

  1. ipfs pin ls now defaults to "all", not "recursive".
  2. ipfs pin rm should respond with Unpinned: HASH, not Pinned: HASH. This is a breaking change.

I don't know how you feel about multiple commits per pull but these were both trivial.

@whyrusleeping
Copy link
Member

Maybe instead of creating a new object for the unpinning, we should change the field name of the existing object to something agnostic like 'Pins' or 'Refs'

@Stebalien
Copy link
Member Author

How about Hash (to be consistent with the add command)?

On March 15, 2016 5:23:01 PM EDT, Jeromy Johnson [email protected] wrote:

Maybe instead of creating a new object for the unpinning, we should
change the field name of the existing object to something agnostic like
'Pins' or 'Refs'


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#2477 (comment)

Steven Allen

@dignifiedquire
Copy link
Member

I don't think making ipfs pin ls default to all is a good idea. The reason being that all is easily very, very large and often much less useful because of that.

@Stebalien
Copy link
Member Author

@whyrusleeping,

Maybe instead of creating a new object for the unpinning, we should change the field name of the existing object to something agnostic like 'Pins' or 'Refs'

I didn't realize this was a list. I changed it to Pins.

@dignifiedquire

That change was made in 29830da (#2208). I just updated the documentation.

@@ -187,7 +187,7 @@ Example:
cmds.StringArg("ipfs-path", false, true, "Path to object(s) to be listed."),
},
Options: []cmds.Option{
cmds.StringOption("type", "t", "The type of pinned keys to list. Can be \"direct\", \"indirect\", \"recursive\", or \"all\". Defaults to \"recursive\"."),
cmds.StringOption("type", "t", "The type of pinned keys to list. Can be \"direct\", \"indirect\", \"recursive\", or \"all\". Defaults to \"all\"."),
Copy link
Member

Choose a reason for hiding this comment

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

Could we remove the 'Defaults to "All"' from the help string and add .Default("all") to the end of the StringOption constructor?

The down later where we check if typeStrFound { we can drop that check and just rely on the code to pick its own default value correctly.

Copy link
Member

Choose a reason for hiding this comment

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

@RichardLitt
Copy link
Member

@whyrusleeping which change? default to all?

@whyrusleeping
Copy link
Member

@RichardLitt relevant to the API:

the json object sent by the pin add and rm commands now has a field of simply Pins instead of Pinned.

It also corrects the documentation to say that "all" is the default instead of "recursive" (no code changes were made for this, all was already the default)

@RichardLitt
Copy link
Member

@whyrusleeping Cool. Will update http api when this is merged.

@whyrusleeping
Copy link
Member

This looks good to me, @Stebalien could you rebase the changes on latest master?

This way, we don't claim to pin when unpinning.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien
Copy link
Member Author

@whyrusleeping Done.

@whyrusleeping whyrusleeping merged commit 8acd87d into ipfs:master Mar 31, 2016
@chriscool
Copy link
Contributor

@Stebalien the change to default to "all" was made following this discussion: #2208 (comment)

To summarize the discussion, the reason is that I added arguments to ipfs pin ls so that one can easily check if a given hash is pinned or not. And when you do ipfs pin ls HASH it is natural to default to "all", so for consistency Juan and myself decided that "all" should always be the default.

@RichardLitt
Copy link
Member

Thanks @chriscool.

@Stebalien Stebalien deleted the pin-fixes branch July 12, 2017 05:22
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.

5 participants