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

py3: use format_exc instead of print_exception #3078

Merged
merged 2 commits into from Jan 7, 2020
Merged

py3: use format_exc instead of print_exception #3078

merged 2 commits into from Jan 7, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 7, 2020

Related: #1818

@ghost ghost requested a review from efiop January 7, 2020 05:40
@Suor
Copy link
Contributor

Suor commented Jan 7, 2020

@MrOutis you left things like cause_tb here, no testing how from exc and implicit chaining works. Why did you close that? It was almost done.

dvc/logger.py Outdated
exc = exc_info[1]
tb = buffer.getvalue()
tb = traceback.format_exc(*exc_info)
Copy link
Contributor

@efiop efiop Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we no longer need cause_tb down below, so tb_list will only contain one entry, so we should get rid of it.

@ghost
Copy link
Author

ghost commented Jan 7, 2020

@MrOutis you left things like cause_tb here, no testing how from exc and implicit chaining works. Why did you close that? It was almost done.

@Suor, it was too big and it addressed changes that were not related to python 3. I'm going over it again.


# NOTE: parsing chained exceptions. See dvc/exceptions.py for more info
while hasattr(exc, "__cause__") and exc.__cause__:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, what about __context__?

Copy link
Author

@ghost ghost Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Suor , __context__? We are not using __context__, and that should be the matter of another PR, I guess 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python 3 automatically assigns __context__ when you raise an exception while handling exception. So if you don't use either from exc or from None you still can show the original cause, this is usually an error in an error handler, which we probably want to see.

@ghost
Copy link
Author

ghost commented Jan 7, 2020

no testing how from exc and implicit chaining works

@Suor , implicit errors don't work with the current implementation.

@Suor
Copy link
Contributor

Suor commented Jan 7, 2020

@MrOutis implicit errors work when you raise exc, so if we are not showing them it means we broke it.

@efiop
Copy link
Contributor

efiop commented Jan 7, 2020

@Suor we didn't have __context__ earlier and didn't have implicit exceptions, nor do we use them in our code (3rd party libs might, be we haven't run into that yet). So no need to address those in this PR.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@efiop efiop requested review from Suor and pared January 7, 2020 17:43
@efiop efiop merged commit e8b0e22 into iterative:master Jan 7, 2020
@ghost ghost deleted the py3-logger branch January 8, 2020 02:38
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

Successfully merging this pull request may close these issues.

2 participants