Repository navigation
Elide uninformative traceback indicators in return and simple assignment statements #99180
Description
Activity
CC @pablogsal
Thanks a lot for raising these @Zac-HD! I think both of this make sense.
I am a bit wary of complicating the already-complex machinery to detect and deal with the ranges even more because the code is becoming a bit complex to maintain but I can give it a go to see how it looks. Also, we have a bunch of in-flight PRs for this area already so we need to land them before attempting anything here.
I have been thinking about all of this and I think the long term solution may be to use colors if possible instead of underlining the ranges with extra characters. This way we save lines and reduce clutter. If colors would be used in the situations you highlight would this have been acceptable?
I think colors would be overridden by the pytest error machinery, as well as much of our logging infra at work, so that would be equivalent to disabling range indicators entirely. That would certainly solve the problem of uninformative indicators, but at the cost of losing useful information too!
Ok, I have this a go and here are my initial thoughts.
- Is quite annoying that we need to parse again all displayed lines. We currently only parse the relevant parts so in
return foo(...)we only parsefoo(...). This forces us to ALSO parse all the displayed lines - The rule for "Skip indicators for lines with a return statement, where every part except the return keyword is indicated" is not that easy for the following reasons:
-
In
return f1("x")("y")if what fails is the second call we currently show:return f1("x")("y") ~~~~~~~^^^^^and I don't think we want to remove that because even if you can deduce it from the context (if only the first call fails the whole line will not highlight) I think it adds clarity and I don't want this to be gone.
-
- We are currently highlighting with two chars even simple calls so is not that easy to discriminate just based on the number of special anchors.
- Same thing applies to simple assignments (they are the same case in reality).
- Is quite annoying that we need to parse again all displayed lines. We currently only parse the relevant parts so in
In any case I created #112670 in case other core devs think otherwise.
@isidentical @lysnikolaou opinions?
- added 4 commits that reference this issue
on Dec 3, 2023 @pablogsal Would it be feasible to only skip the highlights if the highlighted section spans the entire
returnstatement (or the entirerhs) and only one highlighting character is used? This might be a good compromise since it'd still display helpful info when needed (two chars), but the motivating example would still get some benefit from it.@pablogsal Would it be feasible to only skip the highlights if the highlighted section spans the entire
returnstatement (or the entirerhs) and only one highlighting character is used? This might be a good compromise since it'd still display helpful info when needed (two chars), but the motivating example would still get some benefit from it.Yes, but this still will not cover the OP cases because now we use two characters in many places, including calls. For example:
return baz(1) ~~~^^^Hmm, I'm +1 for keeping it as is then. Requiring the user to deduct that the last call of the line is the one failing, if no carets are there, sounds like bad experience.
Another thing to do is explore the entire AST and do this ONLY if there is a single call where the callable is a simple name. But this is more computation at error time which I am not super happy about
Another thing to do is explore the entire AST and do this ONLY if there is a single call where the callable is a simple name.
Oh yes, that achieves what I was trying to achieve above.
But this is more computation at error time which I am not super happy about.
This is true, but it isn't much more computation, and we've always been happy to sacrifice a bit of time for better errors UX, right? Although it's up to you to decide whether it's too much.
Reacted by Zac Hatfield-Dodds and Shantanu- added a commit that references this issue
on May 1, 2024 - added a commit that references this issue
on May 25, 2024
The new traceback indicators can be really nice, though at times also quite verbose. #93883/#93994 by @belm0 improved this situation by skipping the indicators for lines where the entire line was indicated, which helps substantially. I'd like to propose that we take this a small step further:
returnstatement, where every part execpt thereturnkeyword is indicated; e.g.return foo()name = some.more(complicated, call=here)These heuristics are slightly more complicated than "don't indicate the entire line", but I argue that in each of these cases the indicators add little to no information, while compressing the traceback makes it easier to navigate and draws attention to the remaining more-specific indicators.
My motivating example is the traceback reported in pytest-dev/pytest#10466, where I count seven already-elided indicator lines, twelve that would be elided by the return heuristic, seven by the simple-assignment heuristic, and four other lines where the indicators would not be affected by this proposal. I'd even argue that dropping uninformative indicators from a majority (19/30) of traceback lines could be considered a bugfix - indicating on 4/30 lines is quite different to 23/30!
Linked PRs
StackSummary.should_show_caretsprivate #119554StackSummary.should_show_caretsprivate (GH-119554) #119556