R4EPI / epitabulate

Tables for epidemiological analysis
https://R4EPI.github.io/epitabulate
GNU General Public License v3.0
8 stars 0 forks source link

Update {tidyselect} to 1.0.0 #8

Closed zkamvar closed 4 years ago

zkamvar commented 4 years ago

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 commented 4 years ago

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 :disappointed:

zkamvar commented 4 years ago

This was fixed in #9

zkamvar commented 4 years ago

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: https://github.com/R4EPI/sitrep/pull/184), so we can instruct the user to wrap it around any_of() or all_of().