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

Discussion: list_ods_sheets vs ods_sheets #133

Open
chainsawriot opened this issue Aug 9, 2023 · 4 comments
Open

Discussion: list_ods_sheets vs ods_sheets #133

chainsawriot opened this issue Aug 9, 2023 · 4 comments

Comments

@chainsawriot
Copy link
Collaborator

chainsawriot commented Aug 9, 2023

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
Copy link
Contributor

pbrohan commented Aug 9, 2023

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
Copy link
Collaborator Author

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

@jennybc

@jennybc
Copy link
Member

jennybc commented Aug 22, 2023

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
Copy link
Collaborator Author

chainsawriot commented Aug 23, 2023

Thank you very much for the answer @jennybc !

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

  • No action: keep list_ods_sheets()
  • keep ods_sheets() to match readxl; remove the deprecation message
  • change the defaults in v3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants