Catch IOError when loading config, useful message #357
Conversation
| @@ -67,6 +74,8 @@ def pull(dry_run, flavor, interactive, debug): | |||
| ) | |||
| except RuntimeError as e: | |||
| log.critical("Aborted (%s)" % e) | |||
| except SystemExit: | |||
| pass | |||
There was a problem hiding this comment.
I think this defeats the purpose of sys.exit(1) because when you pass on the exception the exit code ends up being 0. Why is this necessary?
There was a problem hiding this comment.
Ooops. This was as otherwise the generic catch all exceptions triggers and you get a traceback which clouds the helpful error message.
There was a problem hiding this comment.
I figured that was the rationale. Is there a way we can suppress the traceback while still throwing a bad exit code?
| except SystemExit: | ||
| pass | ||
| except SystemExit as e: | ||
| sys.exit(e.code) |
There was a problem hiding this comment.
@ryneeverett I think this is the cleaner solution.
|
Why not catch the exception in bugwarrior/config.py's |
|
Nevermind, I guess |
|
This isn't something I'm going to be able to look at much more this week, @ryneeverett or @ralphbean feel free to close this pull request and take a more elegant stab at it. |
If the catch-all exception handler is removed we no longer have to re-catch and re-raise SystemExit's.
This is the only place in which we're logging before setting logging up and it always raises an exception and ends execution. So rather than set up logging twice we can set up logging here. It's lame but it works.
|
I made a PR against @irl's branch. |
Dealing with some issues.
| # configuration file which just failed to load. | ||
| logging.basicConfig() | ||
|
|
||
| log.critical("Could not load configuration. " |
There was a problem hiding this comment.
Should this be log.exception instead? As it stands, the reason for the IOError is hidden. Did it fail due to permissions? Or due to a non-existent file?
There was a problem hiding this comment.
I attempted to address this in irl#3.
This is an attempt at compromise between giving a user-friendly error message on the one hand and giving a technically accurate message that can be used to diagnose the problem.
Fix TypeError and address Ralph's comment about supressing the exception reason.
| @@ -34,6 +33,10 @@ def _try_load_config(main_section, interactive): | |||
| try: | |||
| return load_config(main_section, interactive) | |||
| except IOError: | |||
| # Our standard logging configuration depends on the bugwarrior | |||
| # configuration file which just failed to load. | |||
| logging.basicConfig() | |||
|
No comments for a week so I'd say go ahead and merge this. |
Fixes #354