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

Using the same argument filters as #222 for .import.rio_rdata, .import.rio_xls, and .import.rio_ods #223

Closed
1 of 3 tasks
bokov opened this issue Sep 19, 2019 · 3 comments
Closed
1 of 3 tasks
Milestone

Comments

@bokov
Copy link
Contributor

bokov commented Sep 19, 2019

Please specify whether your issue is about:

  • a possible bug
  • a question about package functionality
  • a suggested code or documentation change, improvement to the code, or feature request

in #222, the intersect() and setdiff() functions were used to generically filter the arguments passed via ... so that only the ones recognized by the underlying import function would get passed to it.

The functions load(), readxl::read_xls(), and readODS::read_ods() underlying .import.rio_rdata(), .import.rio_xls(), and .import.rio_ods() respectively have the same problem.

May I submit a PR/s implementing the same argument-filtering for those functions? If yes, do you prefer a separate PR for each, or grouped together?

@leeper
Copy link
Contributor

leeper commented Sep 21, 2019

Happy to have you tackle this! Your call on whether to submit separately or as one PR.

bokov added a commit to bokov/rio that referenced this issue Sep 23, 2019
bokov added a commit to bokov/rio that referenced this issue Sep 23, 2019
…to 'load()' nor to 'readRDS()' because those underlying functions cannot accept '...' arguments. Instead, the length of '...' is checked and if > 0 a warning is issued. With corresponding tests. As proposed in gesistsa#223
@bokov
Copy link
Contributor Author

bokov commented Sep 24, 2019

Went with separate, so it would be easier to review

@bokov
Copy link
Contributor Author

bokov commented Sep 30, 2019

These PRs are #225 for ODS, #229 for RData (and RDS), and #230 for XLS. Are there further corrections I should make?

Thanks.

leeper pushed a commit that referenced this issue Oct 2, 2019
…to 'load()' nor to 'readRDS()' because those underlying functions cannot accept '...' arguments. Instead, the length of '...' is checked and if > 0 a warning is issued. With corresponding tests. As proposed in #223 (#229)
leeper pushed a commit that referenced this issue Oct 3, 2019
* Added dynamic argument checking for 'readODS:read_ods()' as proposed in #223. Also more detailed tests.
@bokov bokov closed this as completed Oct 4, 2019
@leeper leeper added this to the v0.6 milestone Dec 19, 2019
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

2 participants