handle excel sheets #190

[boshek]

Here is what happens at the moment:

library(bcdata)
#> Attaching package: 'bcdata'
#> The following object is masked from 'package:stats':
#> 
#>     filter

bcdc_get_data('8620ce82-4943-43c4-9932-40730a0255d6')
#> Reading the data using the read_xlsx function from the readxl package.
#> 
#> This .xlsx resource contains the following sheets: 
#>  'Notes'
#>  'Regional Districts'
#>  'Single Detached'
#>  'Multi Unit Homes'
#>  'Purpose Built Rental'
#> Defaulting to the 'Notes' sheet. See ?bcdc_get_data for examples on how to specify a sheet.
#> # A tibble: 0 x 0


bcdc_get_data('8620ce82-4943-43c4-9932-40730a0255d6', sheet = "Single Detached")
#> Reading the data using the read_xlsx function from the readxl package.
#> 
#> This .xlsx resource contains the following sheets: 
#>  'Notes'
#>  'Regional Districts'
#>  'Single Detached'
#>  'Multi Unit Homes'
#>  'Purpose Built Rental'
#> Defaulting to the 'Notes' sheet. See ?bcdc_get_data for examples on how to specify a sheet.
#> New names:
#> * `` -> ...2
#> * `` -> ...3
#> * `` -> ...4
#> * `` -> ...5
#> * `` -> ...6
#> # A tibble: 162 x 6
#>    `Single Detached Homes by~ ...2      ...3  ...4  ...5  ...6                  
#>    <chr>                      <chr>     <chr> <chr> <lgl> <chr>                 
#>  1 Community                  Single D~ <NA>  <NA>  NA    Note:                 
#>  2 <NA>                       2016      2017  2018  NA    * For privacy reasons~
#>  3 100 Mile House             25        16    19    NA    **                    
#>  4 Abbotsford                 336       252   270   NA    Central Saanich inclu~
#>  5 Alert Bay                  *         *     *     NA    Delta includes Tsawwa~
#>  6 Anmore                     18        18    6     NA    Kent includes Agassiz~
#>  7 Armstrong                  35        43    34    NA    Langley includes Lang~
#>  8 Ashcroft                   *         *     *     NA    North Cowichan includ~
#>  9 Barriere                   *         *     12    NA    North Vancouver inclu~
#> 10 Belcarra                   9         *     5     NA    West  Kelowna include~
#> # ... with 152 more rows

^{Created on 2020-05-11 by the reprex package (v0.3.0)}

I think that if a user specifies the sheet argument that bcdc_get_data should be silent. We are trying to warn those who don't know about this. What do you think @ateucher ?

[ateucher]

It's a thing of beauty - great work! Yes, I think for sure we want to suppress the warning if the sheet argument is passed

[boshek]

I hadn't meant to use that language but maybe it should be a warning rather than a message?

[stephhazlitt]

Is it still handy for the user to have have the other sheets printed to the console? e.g. everything else except Defaulting to ...

[boshek]

My thinking is that if someone is already specifying a sheet they likely are aware that multiple sheets exist. That sheet in that resource in that record is the end point of their data discovery journey. At that point it feels we should keep quiet in the console since they don't need any more help. I am just conscious that bcdata is already quite chatty.

[ateucher]

I think both points are good... I think one point in favour of Steph's argument is that if you want a different sheet you need to run the function again without the sheet argument, so could be extra function calls just for that. We could think about adding the info to bcdc_get_record() but that might be one step too far into a bit of a corner case...

[boshek]

I do like the idea of providing users some way of accessing the names of the sheets. Would a function be overkill? bcdc_excel_sheets? If folks have to iterate over a file like that it would be good to provide them with that vector.

[boshek]

I think the conversation of providing a method to programmatically access sheet names could be deferred to a) the first time a user requests it or b) how the process more fully fleshed out during the writing of #192. With that in mind, maybe we could merge this to provide basic functionality?

[boshek]

/document

[stephhazlitt]

I wonder about expanding bcdc_tidy_resources?

[ateucher]

Just one little comment, otherwise looks great!

[boshek]

@stephhazlitt I like that idea too. The problem is that there is no way to know which sheets are present without downloading each excel file. If we had a record like this we'd have to download each file just to generate that table. Then we would have to redownload when we choose the one we wanted. Seems like a lot of downloading. It really highlights why excel files with multiple sheets aren't ideal.

[ateucher]

Following from my last question, the original error message is swallowed by tryCatch, so I added it back in c1a1d80. Now looks like:

> bcdc_get_data('8620ce82-4943-43c4-9932-40730a0255d6', sheet = "foo")
Reading the data using the read_xlsx function from the readxl package.
Error: Reading the data set failed with the following error message:
  Error: Sheet 'foo' not found

The file can be found here:
  '/var/folders/2w/x5wq73f93yzgm7hjr_b_54q00000gp/T//RtmpVj3ASz/bcdata_cf8f2aea4253/filecf8f79a86d27.xlsx'
if you would like to try to read it manually.

[stephhazlitt]

I agree with merging this in now @boshek, good call.

I still like the idea that bcdc_get_data() prints the sheet names even when the user passes a sheet name (and thus likely knows the .xlsx file design). But I am chatty, and especially enjoying chatting w/ my console these days. 🤣