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

Exception handling #206

Closed
ibokuri opened this issue Dec 24, 2018 · 1 comment
Closed

Exception handling #206

ibokuri opened this issue Dec 24, 2018 · 1 comment

Comments

@ibokuri
Copy link

ibokuri commented Dec 24, 2018

There seems to be quite a bit of unhandled exceptions, which doesn't really help the protecting users principle in the design guide. The below for example tries to change the backup directory to a file and then errors out. However, the backup path in .shallow_backup is changed anyways.

screen shot 2018-12-24 at 9 24 38 am

I think program execution should halt gracefully before the users get an ugly traceback and the program's state turns to gobbledygook.

I could start on this if you'd like, I just don't know if you have a preference of how exceptions should be handled. So far, I've just been using the print_red_bold() and print_path_red() functions to indicate the error and then exit the program with a return value of 1 like so:

screen shot 2018-12-24 at 9 23 42 am

edit: I'm an idiot and didn't see some of the handling you already did. Will follow that format.

@alichtman
Copy link
Owner

You bring up some great points here. (On mobile, sorry for the unorganized mess of thoughts.)

I think program execution should halt gracefully before the users get an ugly traceback and the program's state turns to gobbledygook.

100% agreed.

I could start on this if you'd like

This would be awesome + very much appreciated. I'm heading out on vacation at the moment, but as soon as I'm back in a week, I can review any PRs you open.

One approach we may want to take is adding logging with something like loguru or even just the built-in logging module so we can avoid showing scary errors to the user while also not losing any helpful debug info.

I would always prefer giving the user a chance to fix non-critical errors as opposed to returning -1 after printing an error message. Erroring out should be a last resort.

The below for example tries to change the backup directory to a file and then errors out. However, the backup path in .shallow_backup is changed anyways.

Well, that's definitely a logic error... Nice catch. Let's make sure to fix it.

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

2 participants