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

Eliminate C code #129

Merged
merged 4 commits into from Oct 2, 2020
Merged

Eliminate C code #129

merged 4 commits into from Oct 2, 2020

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 26, 2020

In favour of using rlang function with same implementation

In favour of using rlang function with same implementation
@hadley
Copy link
Member Author

hadley commented Sep 26, 2020

Ooops, this makes it impossible to reload rlang

@hadley hadley closed this Sep 26, 2020
@jimhester
Copy link
Member

jimhester commented Oct 1, 2020

What if we called it directly? That wouldn't implicitly load the rlang namespace again e.g. .Call("rlang_env_unlock", env, PACKAGE = "rlang")?

It would be nice to make pkgload R only.

@hadley
Copy link
Member Author

hadley commented Oct 1, 2020

Oh interesting idea. Will that generate an R CMD check error? I'll try it out.

@hadley hadley reopened this Oct 1, 2020
@hadley
Copy link
Member Author

hadley commented Oct 1, 2020

Yes, this seems to work 😄

I tested it with pkgload and rlang and it seems to work. (It does take progressively longer to reload rlang each time, but that appears to be the same with CRAN pkgload)

R/utils.R Outdated
@@ -182,5 +182,6 @@ single_quote <- function(x) {
}

unlock_environment <- function(x) {
.Call(unlock_environment_, x)
call <- .Call # confuse R CMD check
call("rlang_env_unlock", x, PACKAGE = "rlang")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is better, but an alternative way around it is

get(".Call")("rlang_env_unlock", x, PACKAGE = "rlang")

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's slightly nicer

@jimhester jimhester merged commit b57e065 into master Oct 2, 2020
@jimhester jimhester deleted the no-c branch October 2, 2020 13:07
@jimhester
Copy link
Member

Hey! Thanks!

@QuLogic
Copy link

QuLogic commented Feb 24, 2021

Can remove NeedsCompilation: yes from DESCRIPTION now? Though it's not actually there right now, it somehow ends up in the DESCRIPTION on CRAN.

@hadley
Copy link
Member Author

hadley commented Feb 24, 2021

@QuLogic CRAN adds that field; it's not something we control.

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.

None yet

3 participants