-
-
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
Enable REPL to offer to install missing packages if install hooks are provided #39026
Changes from 2 commits
ba8e166
086a556
35ef519
1c12fab
86cc121
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -776,11 +776,46 @@ find_hist_file() = get(ENV, "JULIA_HISTORY", | |
|
||
backend(r::AbstractREPL) = r.backendref | ||
|
||
# Allows an external package to add hooks into the code loading. | ||
# The hook should take a Vector{Symbol} of package names and | ||
# return true if all packages could be installed, false if not | ||
# to e.g. install packages on demand | ||
const install_packages_hooks = Any[] | ||
|
||
function eval_with_backend(ast, backend::REPLBackendRef) | ||
if !isempty(install_packages_hooks) | ||
check_for_missing_packages_and_run_hooks(ast) | ||
end | ||
put!(backend.repl_channel, (ast, 1)) | ||
return take!(backend.response_channel) # (val, iserr) | ||
end | ||
|
||
function check_for_missing_packages_and_run_hooks(ast) | ||
mods = modules_to_be_loaded(ast) | ||
filter!(mod -> isnothing(Base.identify_package(String(mod))), mods) # keep missing modules | ||
if !isempty(mods) | ||
for f in install_packages_hooks | ||
f(mods) && return | ||
end | ||
end | ||
end | ||
|
||
function modules_to_be_loaded(ast, mods = Symbol[]) | ||
if ast.head in [:using, :import] | ||
for arg in ast.args | ||
if first(arg.args) isa Symbol # i.e. `Foo` | ||
push!(mods, first(arg.args)) | ||
else # i.e. `Foo: bar` | ||
push!(mods, first(first(arg.args).args)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might need to be made more robust. I don't think this works for something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't think about either of those. I'll investigate and add tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the |
||
end | ||
end | ||
end | ||
for arg in ast.args | ||
arg isa Expr && modules_to_be_loaded(arg, mods) | ||
end | ||
return mods | ||
end | ||
|
||
function respond(f, repl, main; pass_empty::Bool = false, suppress_on_semicolon::Bool = true) | ||
return function do_respond(s::MIState, buf, ok::Bool) | ||
if !ok | ||
|
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 should probably run in the backend. It could even use the existing
repl_ast_transforms
hook (not a "transform" per se, but no big deal).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. Moved to the backend, but given that the
modules_to_be_loaded
parser resides here, it seemed like a misuse to place that onrepl_ast_transforms
given that if IJulia/Pluto etc. wanted to do a different install hook they'd probably want to go after the modules have been identified bymodules_to_be_loaded
so keepinginstall_packages_hooks
would be necessary anyway. These use models are a bit imaginary though.. my thinking might be off