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

move REPL to stdlib #25544

Merged
merged 2 commits into from
Jan 21, 2018
Merged

move REPL to stdlib #25544

merged 2 commits into from
Jan 21, 2018

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Jan 13, 2018

This moved REPL to the stdlib. Since clearly Base needs to access the REPL to start it, I put a Ref where REPL puts itself in when initializing the module.

TODO:

  • Decide what to do with Base.active_repl and Base.active_repl_backend.
  • Decide if the "Interact with Julia" manual should be moved to the REPL standard library. Probably.
  • Should the file replutil.jl be renamed (it is still in Base)? It defines how e.g. errors are shown should not be REPL specific?

Comments?

@KristofferC KristofferC added stdlib:REPL Julia's REPL (Read Eval Print Loop) kind:excision Removal of code from Base or the repository labels Jan 13, 2018
@KristofferC KristofferC force-pushed the kc/repl_stdlib branch 2 times, most recently from 1a992f3 to 177ec3f Compare January 13, 2018 15:52
@JeffBezanson
Copy link
Sponsor Member

Yes, I think replutil.jl should stay in Base and be renamed maybe showerror.jl. At the top there are a bunch of general non-error show methods that could be in show.jl or other files.

@JeffBezanson
Copy link
Sponsor Member

Hm, we already have arrayshow.jl and methodshow.jl, so I guess make that errorshow.jl.

@KristofferC KristofferC force-pushed the kc/repl_stdlib branch 6 times, most recently from 79ef208 to 30588cf Compare January 14, 2018 11:52
@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Jan 14, 2018

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 Base.active_repl and Base.active_repl_backend be moved to the REPL module?

@JeffBezanson
Copy link
Sponsor Member

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.

@KristofferC
Copy link
Sponsor Member Author

But moving atreplinit makes sense? Or is that also Base functionality?

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Jan 15, 2018

Thinking about it, I sort of want to move back atreplinit to Base. It is not a function specific to the current REPL implementation. It is just stuff that runs when any REPL is loaded, could be the stdlib one, could be a package REPL. Opinions?

Although, it makes is a bit hard to figure out where to put the documentation for it.

@JeffBezanson
Copy link
Sponsor Member

That sounds fine to me.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Jan 17, 2018
@JeffBezanson JeffBezanson added the stdlib Julia's standard library label Jan 17, 2018
@KristofferC KristofferC force-pushed the kc/repl_stdlib branch 3 times, most recently from 4bf56e0 to ce916f9 Compare January 19, 2018 12:06
@KristofferC KristofferC changed the title WIP: move REPL to stdlib move REPL to stdlib Jan 19, 2018
@KristofferC
Copy link
Sponsor Member Author

Should be good to go assuming CI passes. Freebsd builder seemed to stall during building? @iblis17

@iblislin
Copy link
Member

I think it's file testsuit again. I'm rebuilding it.

@KristofferC
Copy link
Sponsor Member Author

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
Copy link
Member

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?

Copy link
Sponsor Member Author

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)
Copy link
Member

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 required ?

Copy link
Sponsor Member Author

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]))
Copy link
Member

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 :)

Copy link
Sponsor Member Author

@KristofferC KristofferC Jan 21, 2018

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}()
Copy link
Member

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).

Copy link
Sponsor Member Author

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"
Copy link
Member

@rfourquet rfourquet Jan 20, 2018

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)
Copy link
Member

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

@KristofferC KristofferC force-pushed the kc/repl_stdlib branch 3 times, most recently from 3bead2d to 78730ba Compare January 21, 2018 15:21
@KristofferC
Copy link
Sponsor Member Author

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.

@KristofferC KristofferC merged commit 3843259 into master Jan 21, 2018
@KristofferC KristofferC deleted the kc/repl_stdlib branch January 21, 2018 19:51
@KristofferC KristofferC added the needs news A NEWS entry is required for this change label Jan 21, 2018
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:excision Removal of code from Base or the repository stdlib:REPL Julia's REPL (Read Eval Print Loop) stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants