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

Update {tidyselect} to 1.0.0 #8

Closed
zkamvar opened this issue Feb 10, 2020 · 3 comments
Closed

Update {tidyselect} to 1.0.0 #8

zkamvar opened this issue Feb 10, 2020 · 3 comments

Comments

@zkamvar
Copy link
Member

zkamvar commented Feb 10, 2020

The problem

Last week, I had noticed that both epibuffet and sitrep were failing with following error:

 ── 1. Failure: survey---adding strata works (@test-tab_funs.R#267)  ────────────

  `{ ... }` did not produce any warnings.

(see https://travis-ci.org/R4EPI/epibuffet/jobs/642800435#L1057-L1058)

When we look at that test, we see that it's testing to see if a warning about a missing column will pop up:

https://github.com/R4EPI/epibuffet/blob/35520c2b20c391d9caf90b8d4b18f8f8584b269d/tests/testthat/test-tab_funs.R#L267-L269

Analysis

The code responsible for throwing a warning there is:

https://github.com/R4EPI/epibuffet/blob/35520c2b20c391d9caf90b8d4b18f8f8584b269d/R/tab_descriptive.R#L239-L265

This used to work because it was possible to pass a character vector into tidyselect and have it match the columns automagically, but changes in version 1.0.0 have made it so that users must specify tidyselect::all_of(CHARVEC) to make it unambiguous between the name of a character vector and the name of a column.

The behavior of tidyselect::vars_select() has also changed, though. Instead of returning nothing, it will return only what works if we select "strict = FALSE", otherwise it will return an error. Because we got specific feedback that the epis want something that will not only work if they misspell something or call a column that doesn't exist (e.g. a test that may not matter in the current situation), but also warn them what columns were not processed, we need to refactor this to make sure it's cromulent with the new version of {tidyselect}

@zkamvar
Copy link
Member Author

zkamvar commented Feb 10, 2020

At this moment, I'm seriously debating if I should just get rid of the warning because the different methods of selecting columns throw different error messages 😞

@zkamvar
Copy link
Member Author

zkamvar commented Feb 11, 2020

This was fixed in #9

@zkamvar
Copy link
Member Author

zkamvar commented Feb 11, 2020

Additionally, I realize this is not really a problem... the current iteration of tidyselect discourages the use of using a vector of column names to subset the data (as we had done before: R4EPI/sitrep#184), so we can instruct the user to wrap it around any_of() or all_of().

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

1 participant