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

Things we want in v2 #440

Closed
eparis opened this issue May 9, 2017 · 18 comments
Closed

Things we want in v2 #440

eparis opened this issue May 9, 2017 · 18 comments

Comments

@eparis
Copy link
Collaborator

eparis commented May 9, 2017

Talking about labeling something 1.0 (to solve #259) and I hope this issue can be a place for us to put API breaking changes we'd like to see in v2.

@eparis
Copy link
Collaborator Author

eparis commented May 9, 2017

What other API mistakes have we made?

@n10v
Copy link
Collaborator

n10v commented May 9, 2017

I want to merge #284 as fast as we can. BTW it fixes 2nd point.

@n10v
Copy link
Collaborator

n10v commented May 9, 2017

I can't understand 3rd point. Can you please explain what do you mean there?

@eparis
Copy link
Collaborator Author

eparis commented May 9, 2017

Say I call ./rootcmd subcmd subsubcmdtypo what will run is subcmd with "subsubcmdtypo" as part of args. I would like our defaults to not accept random strings like subsubcmdtypo as an arg, and instead do the same "did you mean subsubcmd" hinting that we get from the root cmd.

@n10v
Copy link
Collaborator

n10v commented May 9, 2017

#284 fixes that too.

@Fjolnir-Dvorak
Copy link

Hmm, I would like a autocomplete generation build in the cobra program itself. Could look into that. Should be easy and finished pretty fast.

@pbdeuchler
Copy link

I would love to be able to define flags when I'm instantiating a Command struct, as the current functional declarations breaks code organization quite a bit (I define my commands in one package, which essentially only contains the various command variables, so I have to put the Flag function calls in my main/init func and pass around the variables holding the flag info with closures or just deal with package wide globals and use the cmd.Flags() API)

@n10v
Copy link
Collaborator

n10v commented May 17, 2017

@pbdeuchler Can you please write a code example of your wish, of course if it's possible.

@pbdeuchler
Copy link

pbdeuchler commented May 17, 2017

For sure! Here's some off the cuff psuedo code... I'd be open to still requiring function calls to define the flags (if there's background logic or if the pflags library requires it) as long as I can create them somehow during my command declaration

var rootCmd = &cobra.Command{
	Use:   "run",
	Short: "Makes the world a better place through cli tools",
	Run: func(cmd *cobra.Command, args []string) {},
	Flags: &cmd.FlagDeclaration{
			&cmd.PersistentBoolFlag{Name: "flag", Shorthand: "f", DefaultValue: "foo", HelpText: "Bar"},
	}
}

Where cmd.FlagDeclaration is a slice of interfaces or something, open to suggestions or critiques

@n10v
Copy link
Collaborator

n10v commented May 17, 2017

@pbdeuchler What's the problem to use cmd.Flags().Get* functions? More here.

@pbdeuchler
Copy link

pbdeuchler commented May 17, 2017

No worries, happy to help... To be clear, this is about defining flags, not fetching them. Currently, since flag declarations are essentially methods on an already instantiated command struct I'm forced to call them outside of the command declaration... this plus the fact that often the only closure in my cli project I have complete control over is the main or init functions means that my flag declarations always end up in those two functions (not to mention the docs encourage this). So using the API now I end up where I have one file, main.go, that looks something like this:

package main

import (
	"log"
	internal "github.com/me/mycli/internalpackage"
)

var (
	hostVar     string
)

func main() {
	internal.RootCmd.PersistentFlags().StringVarP(&hostVar, "host", "h", "localhost", "the host to query")

	if err := internal.RootCmd.Execute(); err != nil {
		log.Fatal(err)
	}
}

Several problems here:

  1. I have a random variable that's not being used, since I fetch flags using the pflags Get functions (as you mentioned), and is in a different package from my business logic anyway
  2. I'm defining behavior for my rootCmd in a separate package from where all the other logic for rootCmd is defined
  3. If I have multiple flags, especially multiple flags for multiple commands, the main/init function can get pretty messy pretty fast

If I were able to define flags as part of defining commands I'd be able to end up with much nicer code that's all pleasantly separated by concerns

@n10v
Copy link
Collaborator

n10v commented May 18, 2017

I have a random variable that's not being used, since I fetch flags using the pflags Get functions (as you mentioned), and is in a different package from my business logic anyway

You don't have to use it:

internal.RootCmd.PersistentFlags().StringP("host", "h", "localhost", "the host to query")

I'm defining behavior for my rootCmd in a separate package from where all the other logic for rootCmd is defined

What's the problem to define it in the same package?

If I have multiple flags, especially multiple flags for multiple commands, the main/init function can get pretty messy pretty fast

You can create multiple files (like root.go, another_command.go ...) and define flags in them for an appropriate command.

Sorry for my dumb questions, but I can't actually fully understand the problem. The code example you provided is the normal practice and you should do this way.

@eparis
Copy link
Collaborator Author

eparis commented May 18, 2017

You want a declarative definition of flags in the command declaration. Not the imperative definition required today. I like it. Probably something that could be done without a -v2...

@eparis
Copy link
Collaborator Author

eparis commented May 18, 2017

I notice that your 'declarative' is not really declarative. Would it make and sense to have it be truely declarative? Some sort of thing like:

type FlagDef Struct {
    name string
    short string
    type string
    var interfac
    deault ????
    help string
}

And then a cobra command could take a []FlagDef and would "do the right thing"

@pbdeuchler
Copy link

pbdeuchler commented May 18, 2017

@BoGeM ahhhh, I must have missed that helper function in the pflags docs, much appreciated

@eparis exactly! i'm not too concerned about the exact implementation, but I'd be quite happy with that psuedo struct you have there

@n10v
Copy link
Collaborator

n10v commented May 21, 2017

zsh autocompletion?

@n10v
Copy link
Collaborator

n10v commented Oct 29, 2017

#552

@n10v
Copy link
Collaborator

n10v commented Oct 30, 2017

I would like to start working on 2.0.0 version this week and opened a new milestone for it. If you have some feature requests/bug reports/any other enhancements for 2.0.0, please open the separate issue with detailed description.

/cc @eparis

@n10v n10v closed this as completed Oct 30, 2017
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

No branches or pull requests

4 participants