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

Non-default alternative import paths, should we keep them? #343

Closed
chainsawriot opened this issue Sep 6, 2023 · 9 comments
Closed

Non-default alternative import paths, should we keep them? #343

chainsawriot opened this issue Sep 6, 2023 · 9 comments
Assignees

Comments

@chainsawriot
Copy link
Collaborator

chainsawriot commented Sep 6, 2023

  • import("x.csv", fread = FALSE) -> utils::read.table
  • import("x.txt", format = "fwf", readr = TRUE) -> readr::read_fwf (not in Suggests)
  • import("x.dta", haven = FALSE) -> foreign::read.dta
  • import("x.sav", haven = FALSE) -> foreign::read.spss
  • import("x.xpt", haven = FALSE) -> foreign::read.xport
  • import("x.xlsx", readxl = FALSE) -> openxlsx::read.xlsx see also Switch from openxlsx to writexl #310

Potential impact: boxr (r-box/boxr) lists all of them in their doc

https://github.com/r-box/boxr/blob/608f02ec43a56d3af96b049c72e081543f06b511/R/boxr_read.R#L1-L41

Also uses like

https://github.com/opentargets/mendelian-randomisation/blob/85ab9d121bda40d2173a95b305bd5fb7635bef18/ensid_maps/creating_metabolite_ensid_maps/metab_2.R#L19

https://github.com/yonicd/typeR/blob/66e6996f31961bc8b9aafe1a6a6098327b66bf71/data/genthat_extracted_code/dataverse/examples/files.Rd.R#L34

By Thomas Leeper

https://github.com/jamesdunham/dataverse-client-r/blob/096d85ed0ef586825efb9586f5f8f6c7cd61410b/R/get_file.R#L37

https://github.com/leeper/designcourse/blob/1bf40b9d5731ff77dc53df3998257f86dadb716b/Assignments/Lab4.tex#L44

https://github.com/leeper/Rcourse/blob/884b83976ff19c7b887547d841d260d87165797b/Data/CusackIversenSoskiceAPSR2007.R#L4

@chainsawriot
Copy link
Collaborator Author

If they are not needed, we can still keep the parameters (for a while, maybe until the next major version e.g. v2.0); but mark them as deprecated (using lifecycle and give warnings), and use the default path.

@chainsawriot
Copy link
Collaborator Author

@ijlyttle May I have your opinion on this? How would that impact boxr? Thank you very much!

@schochastics
Copy link
Member

I would love to hear from someone how removing this feature could break something. I think this could be dropped and internally handled without too much effort and I am very much in favor of doing that (given that there really is no use case I am missing)

@ijlyttle
Copy link

Hi all,

I have not been able to pay proper attention to boxr lately (hoping to get back to it), as my company has moved away from Box.

I remember a conversation with @nathancday (boxr co-maintainer), where we discussed how tightly bound boxr::box_read() is with rio::import(). I think our conclusion was that, if writing from scratch, we would not do it that way, but that we have an obligation not to break stuff - it sounds like you are having a similar conversation.

I don't (and didn't) use box_read() myself, and I don't really have a good feel for how it's used out in "the wild". That said, I have a sinking feeling that despite the deprecation warnings, we would find out afterwards...

From a coding standpoint, for boxr, I think it would be a matter of updating the documentation to advertise any new behavior from rio.

Of course, I'd want to hear what @nathancday might have to say (Hi Nate, it's been too long!)

chainsawriot added a commit that referenced this issue Sep 11, 2023
* Introduce `lifecycle` (which is a dependency of `tibble` anyway) to
implement the deprecation of `import(readxl)`.

* Remove the overwriting of existing sheets by using `which`.

Ref #343

But `which` is not yet mapped to `col_names` for export(). #326
@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Sep 11, 2023

There is also one non-default export path

  • export(x, "hello.csv", fwrite = FALSE) -> utils::write.table

The alternative path for xlsx is removed in #359

chainsawriot added a commit that referenced this issue Sep 11, 2023
* Zap `openxlsx` and use `writexl` instead, fix #310

* Introduce `lifecycle` (which is a dependency of `tibble` anyway) to
implement the deprecation of `import(readxl)`.

* Remove the overwriting of existing sheets by using `which`.

Ref #343

But `which` is not yet mapped to `col_names` for export(). #326

* Add tests and update NEWS

writexl is 4x the speed of openxlsx

system.time(export(nycflights13::flights, tempfile(fileext = ".xlsx")))

* Add more tests [no ci]
@chainsawriot
Copy link
Collaborator Author

With #359, we have a template for removing these paths

.import.rio_sav <- function(file, which = 1, haven = lifecycle::deprecated(), to.data.frame = lifecycle::deprecated(), use.value.labels = lifecycle::deprecated(), ...) {
    if (lifecycle::is_present(haven) || lifecycle::is_present(to.data.frame) || lifecycle::is_present(use.value.labels)) {
        lifecycle::deprecate_warn(when = "0.5.31", what = "import(haven)", details = "sav will always be read by `haven`. The parameter `haven` will be dropped in v2.0.0.")
    }
    standardize_attributes(haven::read_sav(file = file))
}

@schochastics
Copy link
Member

@chainsawriot I could take over this task if you dont mind

@chainsawriot
Copy link
Collaborator Author

@schochastics Please! I will focus on the ....

@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Sep 13, 2023

I will do that.

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