Skip to content
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

simplify the active_module implementation and make it a "Base feature" #55392

Closed
wants to merge 1 commit into from

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Aug 6, 2024

The current implementation of active_module is REPL instance specific but it becomes confusing since Base has to try reach out for a REPL to ask it what the active module is. There is also things like a global previous_active_module which shows that the REPL instance specific implementation is a best effort at best.

This simplifies things by making the active_module a Base feature (just a global value) and then anything (including the REPL) can set that.

Should fix #54888, #54780.

@KristofferC KristofferC added REPL Julia's REPL (Read Eval Print Loop) backport 1.11 Change should be backported to release-1.11 labels Aug 6, 2024
@JeffBezanson
Copy link
Member

This doesn't make sense to me, since which module a REPL is eval'ing in is indeed a property of that REPL --- to the extent you can have multiple REPLs, they can be talking to different modules. Meanwhile, Base has a separate notion of a current active REPL (Base.active_repl) so it seems to me the "active module" must be the module of that REPL.

Even better, maybe we can delete active_module and only use the module property of the IOContext.

@vtjnash
Copy link
Member

vtjnash commented Aug 6, 2024

SGTM. It is used in a lot of places that are not associated with the REPL, such as Enums, Docs, InteractiveUtils.varinfo, to just list things in this respository

@@ -65,7 +65,6 @@ show(io::IO, x::Prompt) = show(io, string("Prompt(\"", prompt_string(x.prompt),

mutable struct MIState
interface::ModalInterface
active_module::Module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we should delete this, as some REPL instances might not want to use the global active_module. Or could be nullable?

@KristofferC
Copy link
Member Author

Even better, maybe we can delete active_module and only use the module property of the IOContext.

That sounds nice indeed but the following places do not take an IO:

  • varinfo
  • doc!
  • dump

@KristofferC KristofferC mentioned this pull request Aug 8, 2024
68 tasks
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Aug 26, 2024
@giordano giordano deleted the kc/active_mod_repl branch October 7, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base.active_module() design is very badly broken on master
3 participants