-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add stacktrace colors to showing MethodError
s and method lists
#45069
Conversation
I do expect tests to fail, mostly due to doctests. I just wanted to have CI churn away at finding them instead of my laptop 😅 I'll fix them as they occur and after some feedback about the general layout/design. |
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.
In response to your request for comments on Zulip
I like how you spotted and fixed the inconsistency between using @
and at
in the stacktrace versus the error message.
I was made aware of #40251 and now I'm thinking about unifying the two, since they're (in part) about the same exact thing: making methods & their uses easier to read when displayed. |
29ddf22
to
0488abc
Compare
Alright, I incorporated #40251 (with permission) into this PR, since that one kinda stalled and this one also touches method printing (though at a different place). I'll update the images at the top post. I think this should be about it, maybe just some very minor things and this should be good to go! |
MethodError
sMethodError
s and method lists
Ah of course, doctests - those will have to wait for tomorrow. Trying to fix those results in ugly expanded stacktraces to be printed, which I'll have to investigate why that happens.. I didn't touch any logic regarding that, I don't think. |
While fixing the failing tests, I noticed that the ambiguity errors have their own code path here - so I'm changing those too. |
5f14da8
to
1cc22a5
Compare
Alright, the only thing that's missing now should be the doctests (for real this time). I've also added a NEWS.md entry and rebased onto current master. I'm still not 100% sure how to fix the doctests best, without someone in the future running into having to specifically exclude the non-existence of |
Fix error printing issues Incorporate method list printing Fix almost all test cases Remove complete inner module printing from tests This is to be consistent with stacktrace printing, which now shows the topmost module that isn't `Main`, to make it easier to identify packages in stacktraces. Contract home dir in method printing as well Fix now contracted user dir paths Fix failing ambiguity test cases as well as other failing test cases Fix missing newline in ambiguity error
…lists and MethodErrors. Remove bad detection of "required" keyword arguments Fix missing line information for methods without a source file Fix InteractiveUtils tests
Fix last remaining reflection test
26880a3
to
594a9eb
Compare
Alright, I think the doctests are fixed now, so this should be good to go once CI agrees. I took the liberty to squash the commits already, to keep history cleaner. |
594a9eb
to
95be1a4
Compare
Since this has been agreed on by triage, can this be merged soon-ish? It'd be unfortunate if this stalls and I have to keep fixing new doctests/other tests.. |
LGTM |
9dfc9ce
to
2dc4e15
Compare
2dc4e15
to
2aa6fb5
Compare
Apologies for the force pushes after you added the |
The Apple Darwin failure seems unrelated:
as is the PowerPC build failure:
No idea what's going on with the |
I have to say, I'm not a fan of the choice to have variable names be greyed out. To quote past me in #40251
|
The goal here was to make Perhaps this could be done in a future "customize all the colors" REPL theming PR, like suggested in #41435? |
…iaLang#45069) * Add color pass to MethodError showing, like in stacktraces Co-authored-by: Sukera <[email protected]>
This basically takes the module coloring logic from printing stacktraces and applies it to method error printing as well. Also contains some formatting to make the "closest candidates" stand out more by having an empty line precede & follow that block.
Before (master)
After (This PR)
Code for producing the above
example.jl
subdir/subdir2/module.jl
The one thing that kind of bothers me though is that the module coloring between theThe colors between method printing, stacktraces andMethodError
message and the followed stacktrace is not consistent - they're done seperately & in isolation and there's no easy way to "pass on" the existing coloring that's built inMethodError
.MethodError
are now also consistent. There may be a race condition on the underlyingIdDict
, should two tasks set a color at the same time. Since only one printing call would be affected and it's a non-functional part, I'd say this is fine.Opinions, improvement ideas etc. are welcome! I've been bothered by how hard
MethodError
s are to read for a long time now, and I think unifying the printing with stacktraces is a good first step to improve this.Closes #40251 Closes #40913 Closes #30110 Closes #36129