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

Remove depency on env_unlock() #286

Merged
merged 4 commits into from
Jun 28, 2024
Merged

Remove depency on env_unlock() #286

merged 4 commits into from
Jun 28, 2024

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Jun 26, 2024

Closes #276.
Progress towards r-lib/rlang#1705.

  • The reset argument of load_all() is now deprecated. Its purpose was to reuse the namespace instead of creating one from scratch. But this code path is not the default, not well tested, and required unlocking the namespace which is no longer possible on recent versions of R.

  • We no longer hot-patch :: in detached namespaces by default. The patched version redirected self-calls via :: to the detached namespace rather than the currently loaded namespace. This is only needed in very special cases, such as packages containing both native code and standalone files. If the patched :: is needed, just assign it on load:

    on_load(`::` <- base::`::`)

    This is not documented because AFAIK only rlang needed this behaviour.

@lionel- lionel- requested a review from hadley June 26, 2024 10:19
@lionel- lionel- force-pushed the feature/env-unlock branch from b9c2e59 to ab70cc6 Compare June 26, 2024 10:28
@lionel-
Copy link
Member Author

lionel- commented Jun 27, 2024

rlang side at r-lib/rlang#1720

rlang::env_unlock(ns)

if (!env_has(ns, "::")) {
return()
Copy link
Member

Choose a reason for hiding this comment

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

It feels like it would be useful to log this in some way, but if you think it only affects rlang, it's probably fine as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only used in rlang AFAIK and even there I think we ran into other issues that prevented us from using :: in some standalone files. So I prefer to keep this feature low-key, and a warning would affect all pkgload users. But you're right that if this feature does end up being needed by someone, they will have a very hard time to discover this fix. We can revisit if someone reports an issue about this.

@lionel- lionel- merged commit 6ebcb36 into main Jun 28, 2024
11 checks passed
@lionel- lionel- deleted the feature/env-unlock branch June 28, 2024 10:11
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.

Remove env_unlock() usage
2 participants