ropensci / readODS

Read ODS (OpenDocument Spreadsheet) into R as data frame. Also support writing data frame into ODS file.
https://docs.ropensci.org/readODS/
Other
55 stars 22 forks source link

Discussion: `list_ods_sheets` vs `ods_sheets` #133

Open chainsawriot opened 11 months ago

chainsawriot commented 11 months ago

Following up #131, I think it is better to leave this for the community to decide. The interim solution so far is to keep ods_sheets at least until v3. But what should we do for v3?

list_ods_sheets conforms to the so-called Tidy Design Principles on function names. It's a verb. All other functions (although not many) in this packages are verbs: read_ods and write_ods.

However, readxl::excel_sheets violates the Tidy Design Principles, even though readxl is part of Tidyverse. If readODS emulates readxl, we should keep ods_sheets. An additional argument is reverse dependencies: several packages use ods_sheets.

Looking beyond readxl, DBI uses dbListTables. There are also base functions such as list.files, list.dirs.

pbrohan commented 11 months ago

I suspect that we will have the same issues with list_ods_sheets as with ods_sheets, in that the former has become part of too many reverse dependencies.

While I agree that it might actually be better to keep ods_sheets as it lines up with how readxl does things, it's been a good while that users have been told that they should use list_ods_sheets rather than ods_sheets, and it will be a confusing backpedal if they are now told to use ods_sheets again. It would also be nice if whichever is the default were able to have include_external_data=FALSE as a default argument, which we may struggle to do with ods_sheets.

I would vote for the unfortunate compromise of keeping both and removing the warning from ods_sheets, as current users will be used to list_ods_sheets, while new and legacy users will expect ods_sheets. Maybe we could switch the default flag for include_external_data in v3 so that it behaves as users expect (and so have a warning that says that this will change)?

chainsawriot commented 11 months ago

A crazy idea is to invite the readxl developers to add list_excel_sheets(); but is it a dream?

@jennybc

jennybc commented 11 months ago

readxl is in a very quiescent phase right now. So, between that and general lack of a compelling reason to fiddle with it, I think it's unlikely you'll see such a change in readxl.

chainsawriot commented 11 months ago

Thank you very much for the answer @jennybc !

With the answer, I think we should follow @pbrohan 's plan