-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Added 'pin ls' Command #364
Conversation
@@ -32,7 +33,7 @@ on disk. | |||
cmds.StringArg("ipfs-path", true, true, "Path to object(s) to be pinned"), | |||
}, | |||
Options: []cmds.Option{ | |||
cmds.BoolOption("recursive", "r", "Recursively pin the object linked to by the specified object(s)"), | |||
cmds.BoolOption("R", "Recursively pin the object linked to by the specified object(s)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mappum why the change? i think -r, --recursive
were good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we have global options with those names (for recursive file adding), and we error if we have a name collision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mappum i think that's a design problem the commands lib has to handle. -r
is a well established flag to mean recursive. clients need to be able to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can solve it by adding a Recursive
modifier to commands.Option
, to specify an option that tells the commands lib to allow recursive files (kind of like how we already have one to enable recursive files for a certain argument).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
The commands library could have a set of special options which you can use, i.e. cmds.OptionRecursivePath
, which the command library knows to handle specially. (This could be the same for the encoding option.) So, instead of being present everywhere, -r
is only there when you ask for it. (encoding should still be everywhere)
e.g. as a user:
var cmdAdd = &cmds.Command{
Options: []cmds.Option{
cmds.OptionRecursivePath,
...
},
}
var cmdRefs = &cmds.Command{
Options: []cmds.Option{
cmds.StringOption("r", "recursive", ...),
...
},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll implement that.
Good command idea! Would be great to include an overview of how the command will work in the PR, like this one: #229 (comment) |
…ence to the option definiton
…ptionRecursivePath
BTW, this is ready for merge if you want to give one last look. |
|
||
if typeStr != "all" && typeStr != "direct" && typeStr != "indirect" && typeStr != "recursive" { | ||
return nil, cmds.ClientError("Invalid type '" + typeStr + "', must be \"direct\", \"indirect\", \"recursive\", or \"all\"") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch typeStr {
case "all", "direct", "indirect", "recursive":
default:
return nil, cmds.ClientError("Invalid type '" + typeStr + "', must be one of {direct, indirect, recursive, all}")
}
LGTM! ready to merge |
This PR adds a
ipfs pin ls
command, where users can view a list of what is being pinned. It only shows direct pins (keys that have been added withipfs pin add
). Maybe we want it to show the indirect/recursive pins as well?