-
-
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
REPL: allow switching contextual module #33872
Conversation
This seems like a good idea. When I read how Juno did this (before Revise even existed) I remember wondering why we don't do this at the REPL.
It makes me wonder if it could just be |
I thought about it too, and I believe I would definitely customize it like that in my startup file. But |
base/client.jl
Outdated
both for evaluating expressions and printing them. | ||
""" | ||
function activate_module(mod::Module=Main) | ||
global active_repl |
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.
This is redundant, no?
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.
Yeah proably, as it's not modified.
e88bb02
to
f14ea54
Compare
Bump; would be cool to make a decision 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.
I was just thinking about this and I agree, this would be very nice!
I guess prompt paste wouldn't work in this mode, like if you try to paste
(Base) julia> throwto
Theoretically, the module prefix could be detected and the result evaluated into the module but that can be done in another PR.
From what I understand the following has to be done:
- Add some tests
- Add something to the docs about this
base/client.jl
Outdated
Set `mod` as the default contextual module in the REPL, | ||
both for evaluating expressions and printing them. | ||
""" | ||
function activate_module(mod::Module=Main) |
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.
Maybe we can make this a bit shorter? Just for convenience if you want to swap module often. Could be just activate
since the module is implicit with the argument type (would also mirror Pkg.activate
?
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.
activate
sounds fine but then it might be preferable to not export it (i.e. call it Base.activate
) as activate
is quite a generic verb?
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 was sure this was in the REPL
stdlib but it is not. I really think it should be REPL.activate(_module)
.
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.
Yeah, I also thought REPL.activate
would be nice after reading your comment, but it updates Base.active_repl
... But sure it's easy enough to call it REPL.activate
:)
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.
Yeah, but that is arguably irrelevant from a user's point of view.
Yes good point. I will look into that eventually. And yes this needs docs, tests and news, but first approval ;-) |
There is another use-case that I forgot to mention: with the proper definitions in your startup.jl (that I will put here if merged), you can kinda emulate the old |
Yeah, that's nice! I miss that. It also makes sense that it now is a REPL feature instead of before where the actual Main module was replaced (which is much more intrusive).
FWIW, the original
I don't think it is a big deal. |
Well, I approve. So now we are two. Should be enough? 😄 |
Alright, I will finish it up then :) |
One question, you activate |
Yes, having played a bit with moving the |
f14ea54
to
29ff6a8
Compare
I updated with few tests, news and docs. I put back I also changed the prompt to More seriously, It would be good if at least a pair of eyes go through the change, which touches part of Julia that I'm not familiar with. Also, I didn't even attempt to update some parts which contain a hard-coded |
I don't get the |
No, you are describing |
Ah, I see now. My bad, |
stdlib/REPL/src/docview.jl
Outdated
@@ -20,12 +20,12 @@ using Unicode: normalize | |||
## Help mode ## | |||
|
|||
# This is split into helpmode and _helpmode to easier unittest _helpmode | |||
helpmode(io::IO, line::AbstractString) = :($REPL.insert_hlines($io, $(REPL._helpmode(io, line)))) | |||
helpmode(line::AbstractString) = helpmode(stdout, line) | |||
helpmode(io::IO, line::AbstractString, mod::Module) = :($REPL.insert_hlines($io, $(REPL._helpmode(io, line, mod)))) |
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.
The tests in REPL calls these functions explicitly so either these need to be given default arguments for mod
or just change the 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.
Oh right thank you, I had only run the repl.jl tests locally.
Yesterday's triage was in favor, but before merging this, @JeffBezanson might want to fix some instances of hard-coded references to Also, as I added support for using the "Alt-mm" ("Alt-m" followed by "m") keybinding to activate the module whose name (possibly with qualification, e.g. |
stdlib/REPL/src/LineEdit.jl
Outdated
isempty(word) && return beep(s) | ||
try | ||
REPL.activate(Base.Core.eval(Base.active_module(), | ||
Base.Meta.parse(word))) |
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.
TODO: add a note that Core.eval
is used, because Mod.eval(e)
would work fine in general but not when Mod
is Core
.
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.
mod.eval
is also (nearly) always wrong, so I'm not sure it makes sense to comment about that in particular
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.
Ok, will not add a comment :) But I thought that mod.eval
is nearly always correct, except for bare modules (hence incorrrect in the general case).
4670c0f
to
92e3d27
Compare
Would be great to get this feature at some point. |
Agreed! Maybe we could merge it early in the 1.7 cycle to have time to play with it before releasing it (or just release it as experimental) |
I just realized that this feature might help alleviate the long-standing
I don't know enough about the internals, and am not sure this is sound: @timholy would |
Yeah, I think that should be OK. While this is only part of the puzzle (interacting packages are still going to struggle with Let me know if want a review and I'll look it over. |
Thank you for your feedback and review offer! But reviewing would not be required now, as there is no |
92e3d27
to
0820eaa
Compare
I rebased (pretty sure I messed up some commits but it will get squashed in the end anyway) this (and made a few tweaks to it). I think this is a really great piece of functionality. While the extra statefulness in Base is slightly annoying, it isn't really worse than hardcoding Main everywhere in my opinion. So I think we can live with that until the whole special case of Main (or now the "active module") goes away. |
I think this is good to go. I've been playing around with it locally for quite a bit and it is very nice. I aim to merge this in a couple of days unless anything new comes up. Comments welcome. |
Is there a way that the VS Code extension can be informed if this happens, i.e. some sort of hook? It would be nice if we could sync the existing UI in VS Code for module selection with the state in the REPL. CC @pfitzseb |
Main
remains the default module in which REPL expressionsare
eval
ed, but it's possible to switch to a moduleMod
viaactivate_module(Mod)
. The default prompt then indicates this,e.g.
(Mod) julia>
.When not using
Revise
, this can be quite useful to work on a module (e.g. a workflow is redefine functionf
from a module, but first a bunch ofimport Mod: ...
and/orusing Mod: ...
have to be typed for internal functions/constants used inf
. With this change, copy-pasting the function from the module definitions into the REPL works out of the box).