-
-
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
Fix Ctrl+Q for stack traces #36344
Fix Ctrl+Q for stack traces #36344
Conversation
Maybe try julia/contrib/generate_precompile.jl Line 69 in bf3ed57
|
a07d52b
to
ffd4fb3
Compare
@tkf What do you think should be done about the helpmode case? The relevant code is in |
I guess we need #34949 to fix help mode? This PR as-is is still a big usability improvement/fix so I think we should just merge this. Do you think it's possible to write a few tests for this? |
It's probably possible to write a test here, but I'm not really familiar with how |
I think it'd be easier to unit-test this than doing an integration test. It's a bit cumbersome to setup a REPL instance, though: julia> term = REPL.TTYTerminal("dumb",IOBuffer(),IOBuffer(),IOBuffer());
julia> repl = REPL.LineEditREPL(term, false);
julia> repl.specialdisplay = REPL.REPLDisplay(repl);
julia> REPL.print_response(repl, (:hello, false), true, false)
julia> String(take!(term.out_stream))
":hello\n" |
#36289 doesn't really buy us a lot here, because |
Ah, I see. So |
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 it's better to set IOContext
outside outstream
. This function sounds like an accessor function and having a side-effect is not great. For example, it'd mean some innocent use like
Line 982 in 5142abf
println(outstream(repl)) |
would clear out last_shown_line_infos
. I think it'd be cleaner if we can keep outstream
as a simple accessor. We've already discussed this in #36344 (comment) but do you think the change I suggested here work? It's just moving what we were doing in display
to print_response
.
Bump. |
93e818c
to
99adf59
Compare
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.
Sorry, my laptop crashed, that's why it took me some time to restore the changes I made. I refactored most of the logic behind the hint message into with_methodtable_hint
to avoid duplicating this code. I also realized that LAST_SHOWN_LINE_INFOS
should probably be lowercase now, since it's no longer a global variable.
stdlib/REPL/test/replcompletions.jl
Outdated
@@ -110,7 +110,7 @@ end | |||
|
|||
let s = "using REP" | |||
c, r = test_complete(s) | |||
@test count(isequal("REPL"), c) == 1 | |||
@test count(isequal("REPL"), c) == 2 |
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 have no clue what's going on here...
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.
Weird, this now breaks the CI test, but was needed locally on my machine for tests to pass 🤔.
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.
Weird, this now breaks the CI test, but was needed locally on my machine for tests to pass
Yeah, it's a bug in the REPL tests.
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.
Does CI run the tests with different options or why isn't this bug present there?
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.
#32377 has some more info.
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, thanks for the pointer!
Just a note that this is somehow freezing up the buildbots :( |
Are they actually frozen? To me, it looked like they finished, but somehow didn't report back correctly to GitHub. Might have been because GitHub was down for a while? |
Yeah it looks like the problem is on GitHub side. For example, |
Yes they're freezing, I go in manually and end them. Here's one that was been running for 21 hours before I killed it: https://build.julialang.org/#/builders/77/builds/1247 |
Do you think that's related to this PR? That's kind of the annoying thing about changing something in REPL – You change some small thing and suddenly some other thing gets silently broken/stuck. 😄 |
Yes, it's been consistent with this PR, unfortunately. Here's another one: https://build.julialang.org/#/builders/77/builds/1281 That being said, the latest version of this seems to have made it through the |
Weird... I don't see how any of these changes could have fixed this. |
I've rebased onto the latest master, let's hope the Windows build still goes through. 😁 |
@staticfloat All tests now passed, so it seems like this should be ok now. @tkf Would you be willing to review? |
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.
LGTM!
@@ -495,6 +484,22 @@ function complete_line(c::LatexCompletions, s) | |||
return unique!(map(completion_text, ret)), partial[range], should_complete | |||
end | |||
|
|||
with_methodtable_hint(f, repl) = f(outstream(repl)) | |||
function with_methodtable_hint(f, repl::LineEditREPL) |
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 this is a clean approach!
@KristofferC Do you want to review it again? If no objections, let's merge this in a few days. |
Bump. |
Thanks for the fix! (and the ping) |
* alternative fix for JuliaLang#36340 * refactor method hint mechanism * make LAST_SHOWN_LINE_INFOS lowercase * implement style suggestion from @KristofferC * fix use of LAST_SHOWN_LINE_INFOS on latest master
This should be a more general fix than #36341. It currently silently gets stuck while generating precompile statements on my machine, but not sure if that's reproducible.
close #36340