Optional exit code support #361

meatballhat · 2016-04-25T22:33:10Z

This began as a merge resolution of #266 with some whitespace formatting fixes in README, exposed definition of default "success" and "error" exit codes, and style change from .*Fn$ to .*Func$ func type names. Based on discussion below, the patch is evolving:

switch from int in return signature to optional custom error type
does it blend?

See changes to CHANGELOG.md for more details.

Closes #266

The function used by BashComplete, Before, After, Action and CommandNotFound have their won type. This makes easier to change/update the API

Now the exit code can be returned by BeforeFn, ActionFn and AfterFn. The `os.Exit` function is not called by this packaged This closes #66 and closes #164

…pi-develop

jszwedko · 2016-04-26T02:54:18Z

README.md

-	}
-	command.Before = InitInputSourceWithContext(command.Flags, NewYamlSourceFromFlagFunc("load"))
-	err := command.Run(c)
+  command := &cli.Command{


jszwedko · 2016-04-26T03:00:05Z

@meatballhat overall 👍, I like the idea of allowing the centralization of error handling.

Did you also consider this approach in comparison to having Actions return an err? I don't have a strong opinion yet, but I was just curious if you had any thoughts.

meatballhat · 2016-04-26T03:14:26Z

tbh I feel weird about Action returning an int, and the switch to multi-returns in general. I'd rather keep single-return error signatures everywhere and provide a helper func to allow folks to bubble up their own exit code, but only if they want to, e.g.:

// ...
app.Action = func(ctx *cli.Context) error {
    // ... bleep bloop
    return cli.NewExitError("invalid blooping", 3)
}

... which we would handle internally a la:

err := app.Action(ctx)
if exitErr, ok := err.(*ExitError); ok {
    // do stuff maybe
    os.Exit(exitErr.Code)
}
// else don't os.Exit...

whaddya think?

jszwedko · 2016-04-26T03:22:50Z

I am a big fan of this approach over the int

meatballhat · 2016-04-27T13:23:53Z

@txgruppi Please jump in and let us know your thoughts! 😻

txgruppi · 2016-04-27T13:31:38Z

@meatballhat I like the idea of having an error that can bubble up the exit code. 😄

…oder

meatballhat · 2016-04-27T14:01:31Z

@txgruppi @jszwedko what are your thoughts on using reflection to ease migration such that folks can continue to use Action = func(ctx *cli.Context) { } but will get a deprecation warning? I'm thinking about anyone who's using this library without version pinning tools.

txgruppi · 2016-04-27T14:05:46Z

@meatballhat for sure it will be great, I believe the great majority of the users don't use any type of versioning for Go packages. Sadly I am one of them 😞

I'm not sure about performance costs of using reflection but I believe it doesn't apply to this package

meatballhat · 2016-04-27T14:08:16Z

@txgruppi yep, I have a few projects kicking around that don't version pin 😺 I seriously doubt this will have any performance impact 🔬

jszwedko · 2016-04-28T00:34:03Z

@meatballhat nice, I really like the use of reflection to support backwards compatibility (still reviewing to leave inline notes).

jszwedko · 2016-04-28T06:01:33Z

app.go

-	Action func(context *Context)
+	Action interface{}
+	// TODO: replace `Action: interface{}` with `Action: ActionFunc` once some kind
+	// of deprecation period has passed, maybe?


jszwedko · 2016-04-28T06:14:44Z

I left one comment that may need action taken, but otherwise this is great. I like that you were able to preserve backwards compatibility (at the cost of type safety, but I think it is acceptable in this case). We can remove support for the deprecated signature in the future.

The only other way I can think of preserving backwards compatibility is to add another field on the struct, but I think this would introduce a more complexity into the code (thus causing bugs) so I think your approach is appropriate.

txgruppi · 2016-04-28T06:26:16Z

app.go

+// ActionFunc or LegacyActionFunc, the func is run!
+func HandleAction(action interface{}, context *Context) error {
+	if reflect.TypeOf(action).Kind() != reflect.Func {
+		panic("given Action must be a func")


Can't this be a returned error?

jszwedko · 2016-04-29T06:02:52Z

app.go

@@ -404,16 +417,16 @@ func HandleAction(action interface{}, context *Context) error {
 	if len(vals) == 0 {
 		fmt.Fprintln(os.Stderr,
 			"DEPRECATED Action signature.  Must be `cli.ActionFunc`")
-		return nil
+		return err


I kinda like return nil here -- return err visually implies to me that it it is not nil.

Aaah, nvm, I think I see now that this would end up being a real error if there is a panic.

Wait, maybe, now I've confused myself. Is there a case where this wouldn't return nil?

I think this branch will only allow nil, yep. I think I was being overly zealous with the defer impl above.

jszwedko · 2016-04-29T17:22:10Z

app.go

@@ -14,11 +14,13 @@ import (
 var (
 	appActionDeprecationURL = "https://github.com/codegangsta/cli/blob/master/CHANGELOG.md#deprecated-cli-app-action-signature"

+	contactSysadmin = "This is an error in the application.  Please contact the distributor of this application if this is not you."


jszwedko · 2016-04-29T17:22:37Z

Doing so limits the ability for users to have only some of their errors cause the application to terminate while allowing others to bubble up. This was originally an adjustment to #361 in #496 to fix #495, but, in hindsight, I believe that the better approach is to recommend the use of `RunAndExitOnError` for this use case (which is undeprecated here). Fixes #565 #594

txgruppi and others added 6 commits July 28, 2015 20:02

Created types for functions

9c0db3f

The function used by BashComplete, Before, After, Action and CommandNotFound have their won type. This makes easier to change/update the API

Added exit code support

49c1229

Now the exit code can be returned by BeforeFn, ActionFn and AfterFn. The `os.Exit` function is not called by this packaged This closes #66 and closes #164

Updated README.md with exit code sample

b79f884

Updated tests to support exit code

1510d7e

Merge branch 'develop' of https://github.com/txgruppi/cli into txgrup…

10c8309

…pi-develop

Rename func type suffixes Fn->Func and add OnUsageErrorFunc

a17c8cf

meatballhat self-assigned this Apr 25, 2016

codegangsta added the status/claimed someone has claimed this issue label Apr 25, 2016

meatballhat added this to the v2.0.0 milestone Apr 25, 2016

meatballhat added the kind/feature describes a code enhancement / feature request label Apr 25, 2016

jszwedko reviewed Apr 26, 2016
View reviewed changes

meatballhat changed the title ~~Merging #266 to add exit code support~~ Optional exit code support Apr 26, 2016

meatballhat added 5 commits April 26, 2016 07:05

Ensure README examples are runnable

b40b627

Switch from multi-return with exit codes to ExitCoder check

b7329f4

Move error types to errors.go

f3e55a0

Encapsulate ExitCoder check into a lil func

f688d47

Making sure examples in README are valid

882dd2c

Merge remote-tracking branch 'origin/master' into txgruppi-develop

23c7b80

meatballhat added 2 commits April 27, 2016 09:31

TRIVIAL Remove trailing whitespace

b2ac061

Docs around exit error behavior, + handling MultiError in HandleExitC…

d48e22a

…oder

Allow for legacy and newer Action func signatures

d45f7c1

jszwedko reviewed Apr 28, 2016
View reviewed changes

txgruppi reviewed Apr 28, 2016
View reviewed changes

meatballhat added 7 commits April 28, 2016 10:15

Cleanups based on feedback in #361

271b56c

Clarifying errors returned from HandleAction + tests

b453bf5

Moving remaining details from #361 description to CHANGELOG.md

c3a6370

TRIVIAL specify *return* signature in deprecation message

7651aa0

Merge remote-tracking branch 'origin/master' into txgruppi-develop

6eb8c82

Merge remote-tracking branch 'origin/master' into txgruppi-develop

61d4175

Ensure MultiError returned when both Before and After funcs given

4cae17c

jszwedko reviewed Apr 29, 2016
View reviewed changes

A few tweaks based on feedback in #361

06c01a4

jszwedko reviewed Apr 29, 2016
View reviewed changes

meatballhat merged commit 663fc0b into master Apr 30, 2016

meatballhat deleted the txgruppi-develop branch April 30, 2016 12:14

codegangsta removed the status/claimed someone has claimed this issue label Apr 30, 2016

jszwedko mentioned this pull request May 3, 2016

recommended way to 'fail' a command #290

Closed

Songmu mentioned this pull request May 13, 2016

change signatures of doMain to follow recent codegangsta/cli mackerelio/mackerel-agent-plugins#227

Merged

tknzk mentioned this pull request May 19, 2016

fix signnatures. codegangsta/cli mackerelio/mkr#54

Merged

jszwedko mentioned this pull request Jan 9, 2017

idea: cod gen to remove the untyped handler #573

Closed

jszwedko mentioned this pull request Feb 4, 2017

How to Run without exiting on error? #565

Closed

jszwedko mentioned this pull request Feb 4, 2017

Do not exit if commands return non-ExitCoder errors #595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional exit code support #361

Optional exit code support #361

meatballhat commented Apr 25, 2016 •

edited

Loading

jszwedko Apr 26, 2016

jszwedko commented Apr 26, 2016

meatballhat commented Apr 26, 2016

jszwedko commented Apr 26, 2016

meatballhat commented Apr 27, 2016

txgruppi commented Apr 27, 2016

meatballhat commented Apr 27, 2016

txgruppi commented Apr 27, 2016

meatballhat commented Apr 27, 2016

jszwedko commented Apr 28, 2016 •

edited

Loading

jszwedko Apr 28, 2016

jszwedko commented Apr 28, 2016

txgruppi Apr 28, 2016 •

edited

Loading

jszwedko Apr 29, 2016

jszwedko Apr 29, 2016

jszwedko Apr 29, 2016

meatballhat Apr 29, 2016

jszwedko Apr 29, 2016

jszwedko commented Apr 29, 2016

Optional exit code support #361

Optional exit code support #361

Conversation

meatballhat commented Apr 25, 2016 • edited Loading

jszwedko Apr 26, 2016

Choose a reason for hiding this comment

jszwedko commented Apr 26, 2016

meatballhat commented Apr 26, 2016

jszwedko commented Apr 26, 2016

meatballhat commented Apr 27, 2016

txgruppi commented Apr 27, 2016

meatballhat commented Apr 27, 2016

txgruppi commented Apr 27, 2016

meatballhat commented Apr 27, 2016

jszwedko commented Apr 28, 2016 • edited Loading

jszwedko Apr 28, 2016

Choose a reason for hiding this comment

jszwedko commented Apr 28, 2016

txgruppi Apr 28, 2016 • edited Loading

Choose a reason for hiding this comment

jszwedko Apr 29, 2016

Choose a reason for hiding this comment

jszwedko Apr 29, 2016

Choose a reason for hiding this comment

jszwedko Apr 29, 2016

Choose a reason for hiding this comment

meatballhat Apr 29, 2016

Choose a reason for hiding this comment

jszwedko Apr 29, 2016

Choose a reason for hiding this comment

jszwedko commented Apr 29, 2016

meatballhat commented Apr 25, 2016 •

edited

Loading

jszwedko commented Apr 28, 2016 •

edited

Loading

txgruppi Apr 28, 2016 •

edited

Loading