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

tidyselect should not be added to global search path #1274

Closed
3 tasks done
dernst opened this issue Jun 16, 2020 · 4 comments
Closed
3 tasks done

tidyselect should not be added to global search path #1274

dernst opened this issue Jun 16, 2020 · 4 comments
Assignees

Comments

@dernst
Copy link

dernst commented Jun 16, 2020

Prework

Description

Package tidyselect is added to the global search path by use of require. I assume requireNamespace would be more appropriate as subsequent calls to tidyselect functions are qualified by their package name.
The problem was tested with the latest github version as of 1 hour ago.

Reproducible example

library(drake)
"package:tidyselect" %in% search()  # correctly outputs FALSE
drake:::drake_tidyselect_cache()
"package:tidyselect" %in% search()  # now incorrectly outputs TRUE

Expected result

The search() path should not have been altered.

Session info

> sessionInfo()
R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 18.04.3 LTS

Matrix products: default
BLAS/LAPACK: /data/home/ernst/.local/openblas/lib/libopenblas_haswellp-r0.3.9.so

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C
 [9] LC_ADDRESS=C               LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base

other attached packages:
[1] tidyselect_1.1.0  drake_7.12.2.9000

loaded via a namespace (and not attached):
 [1] txtq_0.2.0        prettyunits_1.1.1 fansi_0.4.1       crayon_1.3.4
 [5] digest_0.6.25     assertthat_0.2.1  R6_2.4.1          backports_1.1.7
 [9] storr_1.2.1       magrittr_1.5      progress_1.2.2    rlang_0.4.6
[13] cli_2.0.2         data.table_1.12.8 filelock_1.0.2    vctrs_0.3.1
[17] tools_3.6.1       glue_1.4.1        purrr_0.3.4       igraph_1.2.5
[21] hms_0.5.3         parallel_3.6.1    compiler_3.6.1    pkgconfig_2.0.3
[25] base64url_1.4
@wlandau
Copy link
Member

wlandau commented Jun 16, 2020

tidyselect is not required for essential drake functionality, but it is required for helper functions if the user supplies tidyselect syntax, so I put it in Suggests:.

tidyselect (>= 1.0.0),

@wlandau
Copy link
Member

wlandau commented Jun 16, 2020

I still feel this is the right decision because it helps limit the number of strict dependencies of the package. One consequences is that certain functions will try to automatically load tidyselect. The behavior may seem unusual, but it is unavoidable if we want drake to be easy for most users to install.

@wlandau wlandau closed this as completed Jun 16, 2020
@dernst
Copy link
Author

dernst commented Jun 17, 2020

which_clean seems to always attach tidyselect if it is installed (because drake_tidyselect_cache explicitly calls require, which would normally fail CRAN checks so that's why it's wrapped in eval/parse). How would one go about using which_clean without attaching tidyselect other than removing tidyselect?

wlandau-lilly added a commit that referenced this issue Jun 17, 2020
@wlandau
Copy link
Member

wlandau commented Jun 17, 2020

For tidyselect selectors like starts_with() to work normally, either the package has to be attached or we need to import and re-export tidyselect in drake. I was avoiding the latter because it requires tidyselect to be a strong dependency. Switched in 4141c98.

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