-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
logger: use py3 methods to display the exception #3059
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,7 @@ def test_error(self, caplog): | |
with caplog.at_level(logging.INFO, logger="dvc"): | ||
logger.error("message") | ||
|
||
expected = "{red}ERROR{nc}: message\n".format(**colors) | ||
expected = "{red}ERROR{nc}: message".format(**colors) | ||
|
||
assert expected == formatter.format(caplog.records[0]) | ||
|
||
|
@@ -55,7 +55,7 @@ def test_exception(self, caplog): | |
except Exception: | ||
logger.exception("message") | ||
|
||
expected = "{red}ERROR{nc}: message\n".format(**colors) | ||
expected = "{red}ERROR{nc}: message".format(**colors) | ||
|
||
assert expected == formatter.format(caplog.records[0]) | ||
|
||
|
@@ -66,7 +66,7 @@ def test_exception_with_description_and_without_message(self, caplog): | |
except Exception: | ||
logger.exception("") | ||
|
||
expected = "{red}ERROR{nc}: description\n".format(**colors) | ||
expected = "{red}ERROR{nc}: description".format(**colors) | ||
|
||
assert expected == formatter.format(caplog.records[0]) | ||
|
||
|
@@ -77,9 +77,7 @@ def test_exception_with_description_and_message(self, caplog): | |
except Exception: | ||
logger.exception("message") | ||
|
||
expected = "{red}ERROR{nc}: message - description\n".format( | ||
**colors | ||
) | ||
expected = "{red}ERROR{nc}: message - description".format(**colors) | ||
|
||
assert expected == formatter.format(caplog.records[0]) | ||
|
||
|
@@ -95,7 +93,7 @@ def test_exception_under_verbose(self, caplog): | |
"{red}ERROR{nc}: description\n" | ||
"{red}{line}{nc}\n" | ||
"{stack_trace}" | ||
"{red}{line}{nc}\n".format( | ||
"{red}{line}{nc}".format( | ||
line="-" * 60, stack_trace=stack_trace, **colors | ||
) | ||
) | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please explain why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found it excessive, maybe not enough reason to do so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||
"{red}{line}{nc}".format( | ||
line="-" * 60, stack_trace=stack_trace, **colors | ||
) | ||
) | ||
|
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are using
format_exc
. just some reorganization, nothing on the logic changed besides misreading thewhile
loopThere 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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, @efiop , @casperdcl ; I'll take it into account for further PR's 🙂