Add executor #25
Add executor #25
Conversation
There was a problem hiding this comment.
Sorry for taking so long to get back to this.
I am not sure I quite understand what this PR is doing though, it’s lacking examples or explanations. I am concerned it is duplicating the behavior already present in the Marshaler interface, which I have to admit I never realized wasn’t documented.
Please take a look at that and let me know if it is same/similar or why this is different and necessary.
| Execute(args []string) error | ||
| } | ||
|
|
||
| // ParseAndExecute ... |
There was a problem hiding this comment.
These comments are not really documenting anything. Would you mind augmenting them with an actual description or even proper examples??
| ) | ||
|
|
||
| var ( | ||
| //ErrNoCommand This should never happen. |
There was a problem hiding this comment.
This comment should be more in the shape of ErrNoCommand should never be returned.
But more importantly: If it is never supposed to be returned, why return it? Seems like an appropriate place for a panic() imo.
| ErrNoCommand = errors.New("No command specified") | ||
| ) | ||
|
|
||
| //Executer used to allow auto execution of commands. |
| ) | ||
|
|
||
| //Executer used to allow auto execution of commands. | ||
| type Executer interface { |
There was a problem hiding this comment.
The Executer needs proper documentation. I have no idea how to use this or what it does honestly. I’d like a (testable) example here.
I am gonna review the rest of this without really understanding what this is supposed to do. Apologies if I misunderstand something.
| return exe.Execute(args) | ||
| } | ||
|
|
||
| // ParseAndExecuteCustomArgs ... |
| @@ -106,6 +106,7 @@ func newFlagset(name string, structValue reflect.Value, parent *FlagSet) *FlagSe | |||
| } | |||
|
|
|||
| var ( | |||
| //ErrHelpRequest returned when help is requested. | |||
| @@ -187,6 +188,7 @@ func (fs *FlagSet) hasShortFlag(fname string) bool { | |||
| return ok | |||
| } | |||
|
|
|||
| // FlagByName get a flag by name from the flagset. | |||
| @@ -214,11 +216,12 @@ func (fs *FlagSet) MutexGroups() map[string]MutexGroup { | |||
| return r | |||
| } | |||
|
|
|||
| // Prints the FlagSet's help to the given writer. | |||
| // PrintHelp Prints the FlagSet's help to the given writer. | |||
There was a problem hiding this comment.
Nit: Lower-case “P” in “Prints”
| @@ -10,7 +10,7 @@ import ( | |||
| // HelpFunc is the signature of a function responsible for printing the help. | |||
| type HelpFunc func(w io.Writer, fs *FlagSet) | |||
|
|
|||
| // Generates a new HelpFunc taking a `text/template.Template`-formatted | |||
| // NewTemplatedHelpFunc Generates a new HelpFunc taking a `text/template.Template`-formatted | |||
There was a problem hiding this comment.
Nit: Lower-case “G” in “Generates”
| @@ -28,7 +28,8 @@ func NewTemplatedHelpFunc(tpl string) HelpFunc { | |||
| } | |||
|
|
|||
| const ( | |||
| _DEFAULT_HELP = "\xffUsage: {{.Name}} [global options] {{with .Verbs}}<verb> [verb options]{{end}}\n" + | |||
| // DefaultHelp is default help message template. | |||
|
Friendly ping regarding this PR. If I don’t hear anything I’ll consider this abandoned by next week. |
This adds the executor functionality paradigm that I like. The idea being that you can add an execute method to your structs and it will allow the parse command to also execute the command related to the struct that is used for the provided command. Would love some feedback / ways to improve if the current implementation is not understood / liked. Using from my own fork for now.