-
Notifications
You must be signed in to change notification settings - Fork 69
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
Do not panic on runtime errors #153
Conversation
@@ -461,7 +461,8 @@ func (sd *stackediff) mustgit(argStr string, output *string) { | |||
|
|||
func check(err error) { | |||
if err != nil { | |||
panic(err) | |||
fmt.Printf("error: %s\n", err) |
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.
I find the panic stack trace very useful for debugging, but agree that at this point exposing it to the end user is a bit harsh. Can we add a debug flag to print the stack trace when running with debug.
Instead of passing around a cfg object everywhere, in the case of an error you can use the following to the the cfg and check for debug:
gitcmd := realgit.NewGitCmd(config.DefaultConfig())
cfg := config.ParseConfig(gitcmd)
also in spr/main.go there is a debug cmd line arg that can set the debug config.
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.
I went with an environment variable to avoid putting complex logic like parsing a config file into the error handling path (what if that fails?)
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.
This works and does avoid complicated stuff in the error handling path.
If we set the SPR_DEBUG variable in spr/main.go in the Before func around line 107 where is parses the debug flag, then the debug flag will work as well.
88858e3
to
fa3cf29
Compare
ef49241
to
3d8deb9
Compare
Panics are usually reserved for programming errors (i.e. a bug in the code). This code path is frequently hit for user errors, like running "git amend" with an empty staging area. commit-id:e5019250
3d8deb9
to
85862b4
Compare
@@ -461,7 +461,8 @@ func (sd *stackediff) mustgit(argStr string, output *string) { | |||
|
|||
func check(err error) { | |||
if err != nil { | |||
panic(err) | |||
fmt.Printf("error: %s\n", err) |
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.
This works and does avoid complicated stuff in the error handling path.
If we set the SPR_DEBUG variable in spr/main.go in the Before func around line 107 where is parses the debug flag, then the debug flag will work as well.
✓ Commit merged in pull request #156 |
Stack: