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

Add REPL hook to prompt to install missing packages, if available in registry #2307

Conversation

IanButterworth
Copy link
Member

Paired with JuliaLang/julia#39026

Adds hooks into REPL to prompt to install packages if missing, and available in registry

src/Operations.jl Outdated Show resolved Hide resolved
@IanButterworth IanButterworth force-pushed the ib/repl_install_missing_prompt branch from 4a268bc to a3c9a07 Compare December 31, 2020 18:17
@IanButterworth IanButterworth force-pushed the ib/repl_install_missing_prompt branch from ab34da5 to 5712114 Compare March 13, 2021 08:02
@IanButterworth IanButterworth force-pushed the ib/repl_install_missing_prompt branch 3 times, most recently from bcd3d9a to 13eada7 Compare April 28, 2021 14:52
@IanButterworth
Copy link
Member Author

The base hook and handler support is now available given JuliaLang/julia#39026 has been merged.

The formatting in this PR now looks like

Screen Shot 2021-04-28 at 10 50 55 AM

@fredrikekre
Copy link
Member

CSV vs "CSV" looks a bit strange.

@IanButterworth
Copy link
Member Author

My thinking is that..

On the load side: Julia currently prints the load error message like ERROR: ArgumentError: Package CSV not found in current path:.
So on the load side it's unquoted

On the registry side: Julia/Pkg can't guarantee that the package in the registry is the same package the user wants by name alone, so the "CSV" hints that the user should check that it's the one they want

I've been going back and forth on this though..

@fredrikekre
Copy link
Member

I don't really see how it hints to that. Perhaps the message can be tweaked somehow instead.

@IanButterworth
Copy link
Member Author

How about

julia> using CSV, ImageIO, DoesNotExist
 │ Packages [CSV, ImageIO, DoesNotExist] not found, but packages named [CSV, ImageIO] are available from a registry.
 │ Project `~/.julia/environments/v1.7/Project.toml`
 └ Add [CSV, ImageIO] to the active project and load? (y/n) [n]: 

@fredrikekre
Copy link
Member

Yea that looks good. Perhaps printing the project path is not needed either? Or instead print

julia> using CSV, ImageIO, DoesNotExist
 │ Packages [CSV, ImageIO, DoesNotExist] not found, but packages named [CSV, ImageIO] are available from a registry.
 └ Add [CSV, ImageIO] to the active project (`~/.julia/environments/v1.7/Project.toml`) and retry? (y/n) [n]: 

Perhaps "retry" instead of "load"?

src/Pkg.jl Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Member Author

Ok. Updated to this.
I don't love adding the project to the prompt line. It makes the prompt line much longer which makes it a bit less visually obvious that it's waiting on user input

Screen Shot 2021-04-28 at 1 33 36 PM

@fredrikekre
Copy link
Member

Perhaps it doesn't have to be included at all? I guess in general you either don't care about environments and it will mostly confuse you, or you know about environments, and then you know which one you run in .

src/Operations.jl Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Member Author

In the discussions that lead to the base PR a few people mentioned that showing the active project was important for not confusing the user, given they're not in Pkg mode, and hence don't have the env prefix visible on this action.

If I was to vote I'd go for the original style with a 2nd line devoted to the project

@fredrikekre
Copy link
Member

In the discussions that lead to the base PR a few people mentioned that showing the active project was important for not confusing the user, given they're not in Pkg mode, and hence don't have the env prefix visible on this action.

I see no discussion about this at all in JuliaLang/julia#39026. Honestly I don't quite understand why it would be important. The fact that you typed using CSV seems to suggest that you expected CSV to already be installed in the current environmen (or the stack).

@IanButterworth
Copy link
Member Author

IanButterworth commented Apr 28, 2021

It's from your notes in JuliaLang/julia#26469 (comment) plus slack discussions.

The fact that you typed using CSV seems to suggest that you expected CSV to already be installed in the current environmen (or the stack).

True, but I see another use model for this.. people wanting to install packages without switching to pkg> mode. It's actually quite a slick way of doing it, because all you type is using Foo then y [return] and the package is installed, precompiled quickly and loaded.

I've added the single line project print back. Not to be forceful, but to present it alongside the other changes. Happy to revert if the consensus is something else

Current form:
image

@IanButterworth IanButterworth force-pushed the ib/repl_install_missing_prompt branch from 3295e01 to 392c18c Compare April 28, 2021 23:23
@IanButterworth
Copy link
Member Author

Two more options

Screen Shot 2021-04-30 at 9 23 02 AM

@carstenbauer
Copy link
Member

I was just about to suggest the same: i.e. why not show it in a similar style as in the Pkg REPL mode. So +1 for this solution from me.

@IanButterworth
Copy link
Member Author

It does feel minimal but informative. I've pushed the last one of those two

@IanButterworth
Copy link
Member Author

@fredrikekre what do you think about that last example? Given this isn't being backported, perhaps the formatting could be trialled and adjusted based on use up to 1.7?

@IanButterworth
Copy link
Member Author

The discussion is still open here but I’m thinking it’s better to merge sooner rather than too close to the 1.7 feature freeze, to see how it goes. I think I’ll merge but happily work with any formatting/info feedback. I’ll merge and update Pkg on master in an hour or so if no one has objection

@fredrikekre
Copy link
Member

How about making it look kinda like the Pkg REPL mode syntax?

(@v1) pkg> add CSV ImageIO

@IanButterworth
Copy link
Member Author

Yeah there's a really nice elegance to that, but I think the triage discussion was that the default action when the user hits return should be not to install.

So it would have to be something like

julia> using CSV, ImageIO, DoesNotExist
 │ Packages [CSV, ImageIO, DoesNotExist] not found, but packages named [CSV, ImageIO] are available from a registry.
 └ (@v1) pkg> add CSV ImageIO <- Run and retry? (y/n) [n]: 

@IanButterworth
Copy link
Member Author

Or perhaps just

julia> using CSV, ImageIO, DoesNotExist
 │ Packages [CSV, ImageIO, DoesNotExist] not found, but packages named [CSV, ImageIO] are available from a registry.
 └ (@v1) pkg> add CSV ImageIO ? (y/n) [n]: 

@fredrikekre
Copy link
Member

Yea, I meant something like the latter.

@IanButterworth IanButterworth force-pushed the ib/repl_install_missing_prompt branch from 263c13e to 33ae3a7 Compare May 2, 2021 18:37
@IanButterworth
Copy link
Member Author

With offline help from @fredrikekre we arrived at this, which seems quite nice

Screen Shot 2021-05-02 at 2 34 13 PM

Screen Shot 2021-05-02 at 2 32 33 PM

@IanButterworth IanButterworth force-pushed the ib/repl_install_missing_prompt branch from 33ae3a7 to a6dee3c Compare May 2, 2021 18:48
@IanButterworth IanButterworth merged commit 5cb6828 into JuliaLang:master May 2, 2021
@IanButterworth IanButterworth deleted the ib/repl_install_missing_prompt branch May 2, 2021 21:20
IanButterworth added a commit that referenced this pull request May 3, 2021
* fix reference

* add install prompt tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants