Display errors on browser failure #115

sloum · 2019-12-06T06:06:03Z

sloum commented

2019-12-06 06:06:03 +00:00

Issue #114 notices that we use panic in at least one place. Due to the way that Bombadillo uses the alternate screen buffer panic and other early exit error messages end up getting wiped clean when we clean up the terminal on exit. This PR resolves that in the following ways:

Updated all exits to use cui.Exit
Adds an exit code and message params to ``cui.Exit: cui.Exit(1, "There is an error and this is a very descriptive description")`
Rather than passing errors when something would be worthy of an early exit, we just call exit there
- All early exits have an appropriate exit code associated

Verification:
One easy test of this is to change the configlocation value in defaults.go to something invalid: "/" (assuming non-root user running bombadillo), or "". Doing so, I got pretty clear error messages on exit that were visible and displayed properly. Regular exits pass an empty string to cui.Exit and do not print anything (but do exit with 0).

Issue #114 notices that we use panic in at least one place. Due to the way that Bombadillo uses the alternate screen buffer panic and other early exit error messages end up getting wiped clean when we clean up the terminal on exit. This PR resolves that in the following ways: - Updated all exits to use `cui.Exit` - Adds an exit code and message params to ``cui.Exit`: `cui.Exit(1, "There is an error and this is a very descriptive description")` - Rather than passing errors when something would be worthy of an early exit, we just call exit there - All early exits have an appropriate exit code associated **Verification:** One easy test of this is to change the `configlocation` value in `defaults.go` to something invalid: `"/"` (assuming non-root user running bombadillo), or `""`. Doing so, I got pretty clear error messages on exit that were visible and displayed properly. Regular exits pass an empty string to `cui.Exit` and do not print anything (but do exit with `0`).

sloum added this to the 2.1.0 milestone 2019-12-06 06:06:03 +00:00

asdf was assigned by sloum

2019-12-06 06:06:03 +00:00

sloum added the

bug

enhancement

labels 2019-12-06 06:06:03 +00:00

asdf commented

2019-12-07 00:19:14 +00:00

A very nice improvement that really improves usability.

I've made some slight wording changes on the error messages, which also seems to have fixed the stalled merge conflict check.

A very nice improvement that really improves usability. I've made some slight wording changes on the error messages, which also seems to have fixed the stalled merge conflict check.

sloum commented

2019-12-07 03:43:56 +00:00

Awesome. That text works better. :) I think this will be very helpful to new users trying to get things to work. Hopefully they dont need to debug issues using these error messages, but without it we end up with things like jboverf's issue where it ran and exited with a 0 but nothing else happened.

Merging in and will create a tag for the patch.

I found out that all tags show up on the releases page, but only tags that get made releases get the text with them and only those ones get the additional moniker "stable" next to them.

Awesome. That text works better. :) I think this will be very helpful to new users trying to get things to work. Hopefully they dont _need_ to debug issues using these error messages, but without it we end up with things like jboverf's issue where it ran and exited with a 0 but nothing else happened. Merging in and will create a tag for the patch. I found out that all tags show up on the releases page, but only tags that get made releases get the text with them and only those ones get the additional moniker "stable" next to them.

sloum closed this pull request

2019-12-07 03:44:09 +00:00

sloum referenced this pull request

2019-12-07 05:42:06 +00:00

Where do error messages from `panic(err) go? #114

Sign in to join this conversation.

No reviewers