search

gesistsa / rio

🐟 A Swiss-Army Knife for Data I/O
http://gesistsa.github.io/rio/
600 stars 76 forks source link

Fix some suboptimal patterns identified by lintr #434

Closed Bisaloo closed 3 months ago

Bisaloo commented 3 months ago

One potential last thing is that find_compress() could use the endsWith() base R function:

https://github.com/gesistsa/rio/blob/588971f7c5558805c1e7efc2f89cc0e1f245a4c7/R/compression.R#L1-L35

But from my (admittedly limited) testing, it seems that NA_character_ is a possible value for f, leading to the creation of a file named NA. This means endsWith() is not a good fit here.

But is it an explicit design choice to allow NA_character_ or an unexpected side effect?

Bisaloo commented 3 months ago

Tests failures are due to https://github.com/r-lib/testthat/issues/1969.

chainsawriot commented 3 months ago

@Bisaloo Once again, thank you very much for the PR. Most of them LGTM. I still have to actually stress test the removal of the setwd(). Those code is not automatically checked and last time I checked, setwd() must still be there. #319

chainsawriot commented 3 months ago

For this, NA_character_ is surely an unexpected side effect and should be excluded. So, I think it's safe to use endsWith() here. I will open another issue related to this.

One potential last thing is that find_compress() could use the endsWith() base R function:

https://github.com/gesistsa/rio/blob/588971f7c5558805c1e7efc2f89cc0e1f245a4c7/R/compression.R#L1-L35

But from my (admittedly limited) testing, it seems that NA_character_ is a possible value for f, leading to the creation of a file named NA. This means endsWith() is not a good fit here.

But is it an explicit design choice to allow NA_character_ or an unexpected side effect?

chainsawriot commented 3 months ago

@Bisaloo Once again, thank you very much for the PR. Most of them LGTM. I still have to actually stress test the removal of the setwd(). Those code is not automatically checked and last time I checked, setwd() must still be there. #319

OK, This one should be fine.

chainsawriot commented 3 months ago

@Bisaloo Please add yourself to Authors@R. Then I will merge this.