-
-
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
move REPL to stdlib #25544
move REPL to stdlib #25544
Conversation
1a992f3
to
177ec3f
Compare
Yes, I think replutil.jl should stay in Base and be renamed maybe |
Hm, we already have arrayshow.jl and methodshow.jl, so I guess make that errorshow.jl. |
79ef208
to
30588cf
Compare
Moved the REPL docs to the stdlib location. The part about colors is potentially a bit out of place but not sure if there is a better place to put it. The last question, should |
587a250
to
cceda76
Compare
I think those can stay in Base; they're Base's way of getting a reference to the current repl, so they're mostly a feature of Base itself. |
But moving |
cceda76
to
de75b19
Compare
Thinking about it, I sort of want to move back Although, it makes is a bit hard to figure out where to put the documentation for it. |
7ac86b8
to
cd6e93e
Compare
That sounds fine to me. |
4bf56e0
to
ce916f9
Compare
Should be good to go assuming CI passes. Freebsd builder seemed to stall during building? @iblis17 |
I think it's |
ce916f9
to
23f83d5
Compare
Rebased, will merge if CI pass because this picks up conflicts very fast. |
base/client.jl
Outdated
active_repl.history_file = history_file | ||
end | ||
# Make sure any displays pushed in .juliarc.jl ends up above the | ||
# REPLDisplay | ||
pushdisplay(REPL.REPLDisplay(active_repl)) | ||
# REPLREFDisplay |
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.
Seems like this one was changed accidentaly?
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.
Indeed
base/sysimg.jl
Outdated
Base.require(:Distributed) | ||
Base.require(:Printf) | ||
Base.require(:Future) | ||
Base.require(:Libdl) |
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.
Those 4 libs are arealy require
d ?
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.
Rebase error, thanks.
@@ -220,7 +220,8 @@ try | |||
Dict(s => Base.module_uuid(Base.root_module(s)) for s in | |||
[:Base64, :CRC32c, :Dates, :DelimitedFiles, :Distributed, :FileWatching, | |||
:Future, :IterativeEigensolvers, :Libdl, :LinearAlgebra, :Logging, :Mmap, :Printf, | |||
:Profile, :Random, :Serialization, :SharedArrays, :SparseArrays, :SuiteSparse, :Test, :Unicode])) | |||
:Profile, :Random, :Serialization, :SharedArrays, :SparseArrays, :SuiteSparse, :Test, | |||
:Unicode, :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.
These module names are sorted alphabetically, it may not be so useful, but why not keep the order :)
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 wanted to keep the precompile statements as close as possible to where they originally were.
base/client.jl
Outdated
@@ -366,7 +373,11 @@ function __atreplinit(repl) | |||
end | |||
_atreplinit(repl) = invokelatest(__atreplinit, repl) | |||
|
|||
# The REPL stdlib hooks into Base using this Ref | |||
const REPLREF = Ref{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.
I find the name REPLREF
a bit confusing, this could suggest that it's a ref to a "REPL object" (not module), what about REPLMOD
? (not a big deal anyway).
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'll change it to REPL_MODULE_REF
.
base/sysimg.jl
Outdated
# PR 25544 | ||
@deprecate_binding REPL root_module(:REPL) true ", run `using REPL` instead" | ||
@deprecate_binding LineEdit root_module(:REPL).LineEdit true ", use `REPL.LineEdit` instead" | ||
@deprecate_binding REPLCompletions root_module(:REPL).REPLCompletions true ", run `REPL.REPLCompletions` instead" |
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.
If REPLCompletions
becomes a submodule of REPL
, what about renaming it to REPL.Completions
? REPL.REPLCompletions
looks redundant (REPLCompletions
may also be better if used without its prefix...).
Also, using
is missing in the message: run `using REPL.REPLCompletions` instead"
base/sysimg.jl
Outdated
Base.require(:Printf) | ||
Base.require(:Future) | ||
Base.require(:Libdl) | ||
Base.require(: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 could as well be inserted according to the pre-existing alphabetical order
3bead2d
to
78730ba
Compare
78730ba
to
d69180c
Compare
This PR has passed PR multiple times now (during 3 days) and just been rebased over and over. Last rebase was completely trivial so I'm just going to merge this. |
This moved REPL to the stdlib. Since clearly Base needs to access the REPL to start it, I put a
Ref
whereREPL
puts itself in when initializing the module.TODO:
Base.active_repl
andBase.active_repl_backend
.replutil.jl
be renamed (it is still in Base)? It defines how e.g. errors are shown should not be REPL specific?Comments?