-
Notifications
You must be signed in to change notification settings - Fork 4
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
Report subprocess error messages #81
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #81 +/- ##
===========================================
- Coverage 98.43% 96.98% -1.46%
===========================================
Files 4 4
Lines 256 265 +9
===========================================
+ Hits 252 257 +5
- Misses 4 8 +4 ☔ View full report in Codecov by Sentry. |
Thanks for the contribution! A quick look seems like this is a reasonable approach and thanks for adding tests. I'll take a closer look soon. |
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.
LGTM after pushing some changes. Take a look and let me know if they look OK to you.
Pointed out a style choice and added your new messages to the tests. Otherwise, LGTM. |
Closes #50. I planned on doing this along with my first PR, but I forgot about it until now.
I appended the error message as-is to the
MadoopError
. This risks making the error messages a bit cluttered though, so any ideas for a cleaner format are welcome.