Skip to content

py3: use format_exc instead of print_exception#3078

Merged
efiop merged 2 commits into
masterfrom
unknown repository
Jan 7, 2020
Merged

py3: use format_exc instead of print_exception#3078
efiop merged 2 commits into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 7, 2020

Related: #1818

@ghost ghost requested a review from efiop January 7, 2020 05:40
@Suor
Copy link
Copy Markdown
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.

Comment thread dvc/logger.py Outdated

exc = exc_info[1]
tb = buffer.getvalue()
tb = traceback.format_exc(*exc_info)
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread dvc/logger.py
tb_list = [tb]

# NOTE: parsing chained exceptions. See dvc/exceptions.py for more info
while hasattr(exc, "__cause__") and exc.__cause__:
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 treeverse: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