Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up.import.rio_xlsx dynamically discovers the formal arguments for read_… #222
Conversation
This comment has been minimized.
This comment has been minimized.
codecov-io
commented
Sep 18, 2019
•
Codecov Report
@@ Coverage Diff @@
## master #222 +/- ##
==========================================
+ Coverage 81.16% 82.43% +1.27%
==========================================
Files 18 18
Lines 860 871 +11
==========================================
+ Hits 698 718 +20
+ Misses 162 153 -9
Continue to review full report at Codecov.
|
|
Thanks for this! Generally looks good. A couple of in-line questions and, can you please edit so code style is consistent - spaces around |
| @@ -320,13 +320,23 @@ function(file, | |||
| } | |||
| if (isTRUE(readxl)) { | |||
| requireNamespace("readxl") | |||
| if ("rows" %in% names(a)) { | |||
| warning("'rows' argument ignored when readxl = TRUE. Use 'range' instead.") | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bokov
Sep 19, 2019
Author
Contributor
The reason is that the code I added a few lines below...
if (length(unused)>0) {
warning("The following arguments were ignored when readxl = TRUE:",
"\n", paste(unused, collapse=', '))
}
...will already issue a warning for any invalid ignored arguments, including but not limited to rows. So if somebody passes it a rows argument, they will get warned twice.
That being said, I'll be happy to put the if statement back in if you want me to.
This comment has been minimized.
This comment has been minimized.
|
I have pushed the requested changes and standing by to your decision about deleted part. While we're at it, quick question-- how do you feel about committing the following not directly related changes either here or in a new PR?
Thanks. |
bokov
requested a review
from leeper
Sep 19, 2019
This comment has been minimized.
This comment has been minimized.
|
@bokov I'm going to merge this. Thanks for the contribution! Happy to have you add an .Rproj file as a separate PR. |
leeper
merged commit f618428
into
leeper:master
Sep 21, 2019
bokov commentedSep 18, 2019
…xlsx or read.xlsx and discards all arguments in '...' that don't match, instead of having to catch them individually. Included tests. While I was at it, noticed there were no xls tests, so added a test file and tests (but left .import.rio_xls untouched so far)
See issue #221