-
Notifications
You must be signed in to change notification settings - Fork 23
Add a gap function JuliaPrompt as inverse to GAP.prompt()
#1301
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1301 +/- ##
==========================================
+ Coverage 76.52% 77.56% +1.03%
==========================================
Files 61 28 -33
Lines 4911 1894 -3017
==========================================
- Hits 3758 1469 -2289
+ Misses 1153 425 -728
🚀 New features to boost your workflow:
|
| function () | ||
| local opts; | ||
| opts:= Julia.Base.JLOptions(); | ||
| Julia.Base.run_main_repl( true, (opts.quiet <> 0), Julia.Symbol("no") , (opts.historyfile <> 0) ); |
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.
That's a great start!
But I think it needs work: for starters, we disable/enable certain signal handlers and stuff when entering/leaving the GAP prompt. The same (in reverse) would have to be done here. Probably with a try/catch/finally construct.
I am also somewhat worried about what happens when people nest those: i.e. GAP.prompt() then JuliaPrompt, etc. ... but maybe it'll work fine?
The following is not directly relevant for this PR, nor a request-for-change, just to give an idea where I think we should head: On the long run, I think it would be great if the "GAP prompt" mode was using the Julia REPL functionality. At least until it hits a break loop (re-implementing those might be tricky). I've did some proof-of-concept work on that years ago, perhaps I'll be able to reactivate that one of these days.
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.
But I think it needs work: for starters, we disable/enable certain signal handlers and stuff when entering/leaving the GAP prompt. The same (in reverse) would have to be done here. Probably with a
try/catch/finallyconstruct.
Done (by moving the signal handler stuff into two helper functions, one per direction, that can be then both used in GAP.prompt() and JuliaPrompt(). See 993bf63 (#1301)
I am also somewhat worried about what happens when people nest those: i.e.
GAP.prompt()thenJuliaPrompt, etc. ... but maybe it'll work fine?
This shouldn't be a problem (at least according to my playing around). What is way more harmful is if one nests the same prompt multiple times (i.e. either calling GAP_jl.promp() from gap or GAP.Globals.JuliaPromp() from julia) as our handler adaptation code assumes that the two functions are called in turns. I therefore added some check for this in 34543b4 (#1301), which can already be merged separately to the JuliaPrompt stuff.
Another problem I found was that when using the gap standalone mode a bunch of interface code that is needed for the error handling updates when transferring from gap prompt to julia repl was not even loaded. Thus, from the gap standalone mode it was not possible to start a JuliaPrompt with proper error handling. I gave this a shot by removing some of the special casing of the standalone mode in favor of using more of the generic functionality (which should mostly be equivalent when in GAP mode).
But something here does not work yet. Interrupting julia code works fine when starting julia, changing to GAP and changing back into a nested julia. But it currently does not work when starting the standalone gap.sh and then changing to julia. In this case, the CTRL-C message still comes from GAP
22b5103 to
3331f06
Compare
Resolves #1294.
One minor point that would be nice to have IMO would be overwriting
exit()to just exit the topmost julia session and not terminate the entire julia process.Concerning the gap docstring: I have no idea if this is syntactically fine, nor how to build the manual. The current version was copilot's guess how this should look like.