Conversation
Codecov Report
@@ Coverage Diff @@
## master #302 +/- ##
==========================================
+ Coverage 95.37% 95.39% +0.02%
==========================================
Files 20 20
Lines 1038 1043 +5
Branches 104 103 -1
==========================================
+ Hits 990 995 +5
Misses 27 27
Partials 21 21
Continue to review full report at Codecov.
|
adiroiban
left a comment
There was a problem hiding this comment.
All good. Only minor comments.
Please consider printing the config error to stderr (and any otther error message).
But I think that this can be fixed and then merged without any review.
src/towncrier/build.py
Outdated
| else: | ||
| traceback.print_exc(file=sys.stderr) | ||
| raise | ||
| raise |
There was a problem hiding this comment.
I think that the ConfigError should also be sent to stderr... as it is still an error. Just that is an error that doesn't require a traceback.
Maybe use this to always exit (raise SystemExit) in _main ... but maybe your fix work better with Click.... so is ok.
if isinstance(e, ConfigError):
print(e, file=sys.stderr)
else:
traceback.print_exc(file=sys.stderr)
sys.exit(1)There was a problem hiding this comment.
Seems I forgot to go back and convert this to except ConfigError as e:... But, to your comments, sure, I'll put the error on stderr. For the traceback, I figured that an uncaught exception ends up on stderr anyways. That was the idea with the bare raise (or after adjusting to except ConfigError, nothing) Though, the explicit sys.exit(1) does make the exit code more... explicit. I don't know if that's good for unexpected exceptions or not.
As to Click, it does offer its own version of this mechanism. I started to switch to that briefly before seeing that this was already here. I figured I would leave this focused and consider a full application switch over to Click exceptions separately (if at all). So, I don't think Click needs to be significantly included in consideration how this should be written.
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
| except ConfigError as e: | ||
| print(e, file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
This seems like the simplest change that 1) avoids the traceback for ConfigErrors and 2) doesn't double up the traceback on regular exceptions. Though, admittedly, 2) is actual scope creep for this PR. :] I tried your suggestion which resulted in test failures due to getting SystemExit rather than ValueError exceptions, for example. If you think that is in fact better, I can switch back and update the tests.
|
This can be merged as it is. Thanks. It is already an improvement over the main branch. I prefer SystemExit as for me it's an "expected error" / business as usual type of event. I don't see it as an error, it's just an instruction to end the execution ASAP :) A ValueError is an "unexepected error" that most probably signals a bug. But as mention, this PR can be merged. Thanks! |
|
The 'expected' Well, I guess before merging I have to figure out what happened to codecov... Edit: welp, guess it fixed itself behind my back. |
Fixes #84.