Skip to content

Add restart for package not found errors #1199

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

Merged
merged 2 commits into from
May 10, 2021
Merged

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented May 10, 2021

check_installed() signals error conditions of class rlib_error_package_not_found. The error includes pkg and version fields. They are vectorised and may include several packages.

The error is signalled with a rlib_restart_package_not_found restart on the stack to allow handlers to install the required packages. To do so, add a calling handler for rlib_error_package_not_found, install the required packages, and invoke the restart without arguments. This restarts the check from scratch.

The condition is not signalled in non-interactive sessions, in the restarting case, or if the rlib_restart_package_not_found user option is set to FALSE.

For now the ctor and signal functions for the package-not-found errors are not exported because the error class and signalling protocol are rather intertwined.

@lionel- lionel- requested a review from gaborcsardi May 10, 2021 10:36
Copy link
Member

@gaborcsardi gaborcsardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I'll add support for this in pak, and we should probably add links to the docs, both ways.

@gaborcsardi
Copy link
Member

@lionel- Actually, wait a sec before merging.

@gaborcsardi
Copy link
Member

gaborcsardi commented May 10, 2021

I just checked that requireNamespace() in is_installed() does not throw a packageNotFoundError condition currently, only loadNamespace() does. So that is good, because pak will also catch packageNotFoundError.

However R-core is considering throwing this condition from find.package(), in which case is_installed() will also throw packageNotFoundError, possibly, depending on how they implement this. Hopefully they'll do it in a way that we can still use requireNamespace() to check for installed packages.

I think we don't need to do anything special about this just yet, just wanted to note it here.

So I think this PR is good to merge. :)

@lionel-
Copy link
Member Author

lionel- commented May 10, 2021

hmm chances are they won't signal a condition on requireNamespace() because if a restarting handler is attached that would probably be quite distracting for users, in the case of packages with lots of Suggests features. Thanks for the review!

@lionel- lionel- merged commit ea154c3 into r-lib:master May 10, 2021
@lionel- lionel- deleted the pnf-error branch May 10, 2021 12:26
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.

2 participants