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

Migrate to cobra CLI system #123

Closed
wants to merge 6 commits into from
Closed

Migrate to cobra CLI system #123

wants to merge 6 commits into from

Conversation

mappum
Copy link
Contributor

@mappum mappum commented Sep 24, 2014

Refactored the CLI commands to use cobra. Nothing outside of the main package (the CLI entry point/commands) is changed by this PR.

Addresses #111

  • Now subcommands automatically get a nice help command when running ipfs help <cmd>
  • Both long and short flags can be used, e.g. ipfs add -r or ipfs add --recursive
  • Flags can now be defined as either persistent or local. For example the --config flag is persistent and can be accessed by any subcommand

@btc btc added the status/in-progress In progress label Sep 24, 2014
@mappum mappum changed the title Feat/cobra Migrate to cobra CLI system Sep 24, 2014
}

func lsCmd(c *commander.Command, inp []string) error {
func init() {
CmdIpfs.AddCommand(cmdIpfsLs)
Copy link
Contributor

Choose a reason for hiding this comment

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

for readability, may want to perform registration in a single spot. easier to see everything that's registered.

Copy link
Contributor

Choose a reason for hiding this comment

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

centralization also provides ability to choose which commands to include, exclude

Copy link
Member

Choose a reason for hiding this comment

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

commands should hold their children, as before. sometimes one command may be pointed at by multiple parents. and this should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

(also makes it really easy to break out subcommands into independent packages that can then be used by other tools (say ipfs name and ipns are the same cmd)

@jbenet
Copy link
Member

jbenet commented Sep 24, 2014

silly vendoring complicating CR:

}

func addCmd(c *commander.Command, inp []string) error {
func addCmd(c *cobra.Command, inp []string) {
Copy link
Member

Choose a reason for hiding this comment

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

how does cobra return errors? it just prints them out itself??? (arghhh).

Copy link
Member

Choose a reason for hiding this comment

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

this bothers me.... fork?fork?fork?

@jbenet
Copy link
Member

jbenet commented Sep 24, 2014

thanks @mappum! made comments up there. i think a lot of these changes are bad for how I want to structure the composability of code. particularly:

  • really nice and cleanly separated subcommands
  • full control over how command text displays (i.e. override the default auto-generated stuff)
  • actually returning errors!!
  • auto-generated command listings from help. (try running ipfs commands and ipfs commands help -- which we can later upgraded to generate man pages)

lesser (annoying but whatever) issues:

  • static flags

if cobra doesn't allow us to do this, then the global flags thing is really not worth it. one of the reasons i chose & forked commander was to precisely allow this sort of stuff. I think either:

  1. we patch the functionality we want (mainly persistent flags?) into our own commander package.
  2. fork cobra to make it do what we want it to.

let's wait for thoughts from others, and then either

@mappum
Copy link
Contributor Author

mappum commented Sep 24, 2014

  • The subcommand system still works the same, but with a different API (you can do cmd.AddCommand(subCmd))
  • We can override any of the help/usage texts, either with a whole new function, or a new template
  • Yeah, unfortunately the error handling (or absence of) isn't too great

A better solution might be to work on a command system that supports commands through multiple transports (CLI, HTTP RPC, raw TCP RPC), and multiple encodings, then just build a CLI system that adheres to that Transport interface. That makes a lot of sense, since most of the commands are only boilerplate for making RPC calls.

That would make adding new commands extremely easy since you can just implement it in one place, and not have to make wrapper CLI/HTTP/etc handlers just to call each command.

@jbenet
Copy link
Member

jbenet commented Sep 25, 2014

A better solution might be to work on a command system that supports commands through multiple transports (CLI, HTTP RPC, raw TCP RPC), and multiple encodings, then just build a CLI system that adheres to that Transport interface. That makes a lot of sense, since most of the commands are only boilerplate for making RPC calls.

Yeah, I like the sound of this. Can you sketch out what the interfaces would look like?

That would make adding new commands extremely easy since you can just implement it in one place, and not have to make wrapper CLI/HTTP/etc handlers just to call each command.

Yep. Though the devil's in the details (i.e. CLI cmds will still need independent handlers to ensure that the command makes sense to type as a human, and HTTP handlers may still be needed to ensure things still make sense to curl manually, etc etc. I.e. these abstractions are in general really good, and of course it is critical to put in place a common interface they all use (to ensure no logic gets duplicated), but often the automatic, abstract thing is too general to make sense for the UX. So a custom wrapper handler will most likely still be needed to make things nice and usable by humans)

@jbenet
Copy link
Member

jbenet commented Sep 26, 2014

@mappum @whyrusleeping so what do we do here-- hack our own package to do this? fork cobra to do what we want?

@whyrusleeping
Copy link
Member

I think we can fork it. The changes that need to be made arent huge, and we could even name it something like "Taipan: more dangerous than cobra", or alternatively, since we would be fixing error handling "Anaconda: Probably less dangerous than cobra"

@jbenet
Copy link
Member

jbenet commented Sep 30, 2014

@whyrusleeping haha yes. +9001 to either of those names.

@jbenet jbenet force-pushed the master branch 2 times, most recently from cd43433 to 8f1c12e Compare October 14, 2014 10:19
@mappum mappum closed this Oct 14, 2014
@btc btc removed the status/in-progress In progress label Oct 14, 2014
@btc btc deleted the feat/cobra branch December 18, 2014 15:30
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
Set KeysOnly field of query condition to true for Datastore.Query(), then it will query only keys.
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