logger: use py3 methods to display the exception #3059
Conversation
ghost
mentioned this pull request
ghost
requested a review
from 
|
Does someone remember why do we have three handlers
But I don't get EDIT: It is to not include |
dvc/logger.py
Outdated
| if record.exc_info: | ||
| _, exception, _ = record.exc_info | ||
| cause = exception.__cause__ | ||
| trace = "".join(traceback.format_exception(*record.exc_info)) |
There was a problem hiding this comment.
This should only be formatted under DEBUG if and should somehow include cause traceback. Ideally cause traceback should not be appended, but separated the same as Python does by default.
dvc/logger.py
Outdated
| message=record.msg or "", | ||
| separator=" - " if record.msg and exception.args else "", | ||
| exception=exception, | ||
| cause=": " + str(cause) if cause else "", |
There was a problem hiding this comment.
This should only be shown in DEBUG mode and should not be shown in the message anyway. Let's use Python default presentation.
There was a problem hiding this comment.
@Suor , this would change the previous behavior, I could move that discussion into another issue.
I've my doubts with the way logger is currently implemented and used 😅
| return ( | ||
| "{color}{levelname}{nc}: {description}" "{stack_trace}\n" | ||
| ).format( | ||
| color=self.color_code.get(record.levelname, ""), | ||
| nc=colorama.Fore.RESET, | ||
| levelname=record.levelname, |
There was a problem hiding this comment.
@MrOutis Why do we need to make these changes? Isn't the point of this change to just use py3 interfaces? If so, the only thing that should be affected is _parse_exc. Or am I missing something?
There was a problem hiding this comment.
we are using format_exc. just some reorganization, nothing on the logic changed besides misreading the while loop
There was a problem hiding this comment.
@MrOutis I understand, but it really widens the scope and makes this hard to review when having simple py3 migration in mind. Something might slip by. 🙁
There was a problem hiding this comment.
I agree with @efiop - it's very tempting to fix multiple issues in one go but with a big PR like this it's best to tidy up/reorganise in a follow-up PR.
There was a problem hiding this comment.
got it, @efiop , @casperdcl ; I'll take it into account for further PR's 🙂
| @@ -117,7 +115,7 @@ def test_nested_exceptions(self, caplog): | |||
| "{red}ERROR{nc}: message - second: first\n" | |||
| "{red}{line}{nc}\n" | |||
| "{stack_trace}" | |||
| "{red}{line}{nc}\n".format( | |||
There was a problem hiding this comment.
Could you please explain why \n are getting removed now?
There was a problem hiding this comment.
I found it excessive, maybe not enough reason to do so.
There was a problem hiding this comment.
@MrOutis The newline is needed there, it is part of the message. I imagine you are trying to solve double newline between the exc and "Having any troubles" footer, right? If so, it is probably worth just removing \n there. But again, this is out of scope for this PR, so let's not touch new lines at all, please roll back this change for now.
|
Closing this one in favor of #3078 |
ghost
closed this
| logger.exception("message") | ||
| except DvcException as exc: | ||
| try: | ||
| raise ValueError("third") from exc |
There was a problem hiding this comment.
Need to test how it works without explicit cause. Maybe that is even more important because that is an error in error handler presumably, which we want to fix.
Related: #1818
Did the best that I could.
formatExceptionandformatStackdoesn't work for us because of the way we are displaying the traceback and manipulating the title of the exception.