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

Copying functions from rlang now fixes version on installation #2

Open
JoFAM opened this issue May 24, 2018 · 5 comments
Open

Copying functions from rlang now fixes version on installation #2

JoFAM opened this issue May 24, 2018 · 5 comments

Comments

@JoFAM
Copy link

JoFAM commented May 24, 2018

If you do the following:

typed_as_name <- rlang::enquo

the function code is copied from rlang into your own package at time of installation. That also means that when an updated version of rlang is installed by the user, your function typed_as_name still uses the old rlang::enquo code. The prefered (and documented) way to get around this issue, is to wrap it in a function (see eg explanation in Writing R extensions:

typed_as_name <- function(arg) rlang::enquo(arg)

But in that case typed_as_name won't work any longer due to dplyr::mytate_impl() not being able to process arg correctly. Same happens when you try with

typed_as_name <- function(arg) eval.parent(rlang::enquo(arg))

Given the nature of the function rlang::enquo, this issue is pretty hard to get around. Then again, these rlang functions are unlikely to change so this shouldn't really be a problem in everyday use.

@MilesMcBain
Copy link
Owner

Thanks for letting me know about this! What about setting the functions using the on load hook as an idea?

@MilesMcBain
Copy link
Owner

Although everything will still work if user uses the conversion feature. So maybe it's simplest to do nothing.

@JoFAM
Copy link
Author

JoFAM commented May 24, 2018

Thought about it, but when using .onLoad one should not assume any package except the base package to be on the search path. So pretty sure that this is opening a can of worms you rather keep closed.

I couldn't come up with something either, so "do nothing but keep in mind this might be an issue at one point" would be my choice too.

@lionel-
Copy link

lionel- commented Jun 6, 2018

When you use .onLoad() you can assume any package in your Imports: is loaded (the search path doesn't matter here). This is the right time to populate aliases in your namespace at load time rather than build time. Joris is right that the latter is problematic.

@MilesMcBain
Copy link
Owner

Thanks for the clarification Lionel!

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

No branches or pull requests

3 participants