-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Revert change to traceback repr #7535
Revert change to traceback repr #7535
Conversation
Would be good to have a test to prevent this kind of regression in the future? It looks like |
I did wonder about that, but couldn't think of a test that would catch similar issues without being rather fragile to non-problems like the names of parent directories. Personally I'm OK with relying on low code churn here, but if anyone wants to push a test to this branch be my guest 🙂 |
I think a test is important here, we would just need to use a regex to make the test independent from the actual file path. |
@@ -262,7 +262,12 @@ def __str__(self) -> str: | |||
raise | |||
except BaseException: | |||
line = "???" | |||
return " File %r:%d in %s\n %s\n" % (self.path, self.lineno + 1, name, line) | |||
return " File %r:%d in %s\n %s\n" % ( |
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 think we should show the non-repr of the path here:
return " File %r:%d in %s\n %s\n" % ( | |
return " File %s:%d in %s\n %s\n" % ( |
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 don't think so - Python also quotes the path in its tracebacks:
Traceback (most recent call last):
File "test.py", line 1, in <module>
raise Exception
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.
Ah, though Python uses "
in any case:
File "test"foo.py", line 1, in <module>
so I suppose we could do File \"%s\"
here instead (and drop the str(...)
again?).
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 don't think so - Python also quotes the path in its tracebacks:
Ahh you are right. 👍
so I suppose we could do File "%s" here instead (and drop the str(...) again?).
Right on. 😁
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.
File "test.py", line 1, in
The code seems to (before my oversight) do
File "test.py":1 in
Which looks kinda weird. Should we indeed change it to match the Python format, i.e File "%s", line %d, in %s
?
Though that would break werkzeug as well, so not for this 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.
Oh, I hadn't noticed that. I guess let's just stay with the previous format for now then?
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.
Actually I noticed previously this used '
instead of "
, based on the failure reported in #7534:
traceback = "".join(str(line) for line in baz_exc.traceback)
> assert "bb.py':1" in traceback # a bit different than typical python tb
(Nevermind this, replied to a stale comment hehehe)
@nicoddemus - I've pushed a final small tweak, because This last patch therefore restores the previous behaviour exactly, and adds a comment explaining why we're not matching Python. IMO if we're going to make a change at all, we should do it to exactly match Cpython and be done with it; but that's a topic for later. |
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.
Looks great! Not sure if it needs a changelog entry or not, though?
I think it makes sense because the change was released in 6.0.0rc1, and I gather we will keep the release candidate changelogs in the final version. |
Awesome. 👍 |
One step closer to a final 6.0 then! 🎉 Thanks everyone! |
This PR ammends #7274, undoing an incidental change to the output string and therefore fixes #7534.
cc @nicoddemus @hroncok