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

Let CLI report on exception causes #7210

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Conversation

bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented Nov 30, 2022

Ensure the already existing format_exception_with_cause also works with traceback.TracebackException and use this to provide another format method for CapturedException. This is then used with unexpectedly bubbled up exceptions by the CLI.

Fixes #7163

FTR:

Actually hard to find a case where I know an exception would bubble up and actually have a __cause__ in core.

Making one up like this:

 raise ValueError("something is wrong") from RuntimeError("severe problem")

Would now be:

> datalad wtf
[ERROR  ] something is wrong -caused by- severe problem 

Whereas

 raise ValueError("something is wrong") from RuntimeError

Would lead to

> datalad wtf
[ERROR  ] something is wrong -caused by- RuntimeError

Rational for sparing the class name when there's a message, is taken from the existing implementation used by special remotes:

For each exception in the chain either the str() of it is taken, or the
class name of the exception, with the aim to generate a simple and
comprehensible description that can be used in user-facing messages.
It is explicitly not aiming to provide a detailed/comprehensive source
of information for in-depth debugging.

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Base: 88.90% // Head: 90.47% // Increases project coverage by +1.57% 🎉

Coverage data is based on head (97b384f) compared to base (c868ae9).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 97b384f differs from pull request most recent head f2cb946. Consider uploading reports for the commit f2cb946 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7210      +/-   ##
==========================================
+ Coverage   88.90%   90.47%   +1.57%     
==========================================
  Files         355      355              
  Lines       46686    46676      -10     
  Branches     6352     6352              
==========================================
+ Hits        41506    42230     +724     
+ Misses       5165     4431     -734     
  Partials       15       15              
Impacted Files Coverage Δ
datalad/cli/main.py 82.00% <100.00%> (ø)
datalad/support/exceptions.py 85.00% <100.00%> (+0.13%) ⬆️
datalad/support/tests/test_captured_exception.py 100.00% <100.00%> (ø)
datalad/metadata/extractors/xmp.py 24.07% <0.00%> (-68.52%) ⬇️
datalad/metadata/extractors/tests/test_xmp.py 51.85% <0.00%> (-48.15%) ⬇️
datalad/metadata/tests/test_extract_metadata.py 55.26% <0.00%> (-44.74%) ⬇️
datalad/metadata/extract_metadata.py 69.44% <0.00%> (-30.56%) ⬇️
datalad/runner/protocol.py 83.82% <0.00%> (-13.24%) ⬇️
datalad/cmd.py 83.45% <0.00%> (-10.30%) ⬇️
datalad/downloaders/tests/utils.py 92.30% <0.00%> (-7.70%) ⬇️
... and 46 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Nov 30, 2022
Ensure the already existing `format_exception_with_cause` also works
with `traceback.TracebackException` and use this to provide another
format method for `CapturedException`. This is then used with
unexpectedly bubbled up exceptions by the CLI.

Fixes datalad#7163
@yarikoptic
Copy link
Member

no objections, although would have been nice to see in comparison to how it would look now, but I would just assume that now it would just not say that exception was caused by another exception. so, as such, providing information about chain would might come useful (although without traceback/location information -- of limited immediate use).

@bpoldrack
Copy link
Member Author

I would just assume that now it would just not say that exception was caused by another exception

Yes, w/o the PR causes are not reported.

@bpoldrack bpoldrack merged commit e23154a into datalad:maint Dec 2, 2022
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.17.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI toplevel exception reporting hides "cause"
3 participants