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
Split REPLMode into an package extension called REPLExt #3724
Conversation
ext/PkgREPLMode/PkgREPLMode.jl
Outdated
@@ -1,16 +1,22 @@ | |||
# This file is a part of Julia. License is MIT: https://julialang.org/license | |||
|
|||
module REPLMode | |||
module PkgREPLMode |
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.
module PkgREPLMode | |
module REPLExt |
I really don't like the trend to put the parent package name first and this should bear the Ext name
Also, nit but it's a "package extension" not an "extension package" |
This requires coordination with JuliaLang/julia#52467 |
Would this break usage like this?
|
Yes. Although I think it has been quite clear that this is not the intended use. julia -e 'using Pkg; pkg"status"'
┌ Warning: The Pkg REPL mode is intended for interactive use only, and should not be used from scripts. It is recommended to use the functional API instead. To make it work, they would need to load it as follows.
|
Since it is a macro, could we just add |
I think that breakage is very reasonable tbh just might come up in pkgeval |
Does anything in that code path even use REPL though? Why hinge this functionality on whether REPL is loaded? |
Yes, it is very dependent on the REPL. From master, Pkg.jl/src/REPLMode/REPLMode.jl Lines 459 to 470 in f7f222f
This branch, Lines 458 to 492 in 4a784ae
|
Thats surprising, looks like it is only for the help view though, perhaps it could be refactored. On another note, typically the entry point to the extension code is some type that you pass to the function, but in this case it will just magically work if REPL happen to be loaded, right? |
The
Yes. There are actually hooks on both sides. I just did some refactoring in 03a7da0 to make the initialization a bit more robust. Also there is a hook on the REPL side that needs some modification for this pull request to work. I kept the name PkgREPLMode in REPL because referring to |
Maintaining this branch as other pull requests get merged is going to be a challenge since so many files are affected. |
import Base.string | ||
using REPL.TerminalMenus |
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 probably breaks the RadioMenu
stuff in this file. However, I think that is more or less useless and should be removed.
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.
Is this the only section that is affected?
We could probably create some kind of hook to move that into the extension if we still need it. Perhaps the more pressing thing is developing a test to cover that part of the code.
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.
Addressed in d9138bd
Regarding the Avoiding having to load the REPL for Pkg will be a very nice thing. |
Subsumed by #3777 |
This PR separates Pkg.REPLMode into an package extension called PkgREPLMode.