Writexlsx params #194
Writexlsx params #194
Conversation
still a WIP
* when rownames is TRUE the first column (which already has * a name made) is removed this makes sure that the made names start with X1, X2 rather than X2, X3
* simplifies x[grepl()] to grep(value = TRUE) when appropriate * uses inherits() or is.*() for class checking * formatting
speed is virtually the sample but grep(value = TRUE) less memory allocation than x[grepl()]
Codecov Report
@@ Coverage Diff @@
## master #194 +/- ##
==========================================
- Coverage 65.10% 64.62% -0.49%
==========================================
Files 32 34 +2
Lines 9105 8873 -232
==========================================
- Hits 5928 5734 -194
+ Misses 3177 3139 -38
Continue to review full report at Codecov.
|
| @@ -69,43 +69,47 @@ | |||
| #' } | |||
There was a problem hiding this comment.
why did you modify this file?
There was a problem hiding this comment.
Mostly from adding assert_*()s and while testing for any differences in changing up write.xlsx() as there were a lot of testing code that used the write.xlsx(x, file); read.xlsx(file) process
| @@ -29,40 +29,40 @@ | |||
| #' category = "something" | |||
There was a problem hiding this comment.
same for this file. only whitespace changes?
There was a problem hiding this comment.
Some changes at 386. Not entirely sure what all the white space changes are. I'm using Rstudio 1.4.1103, could there be some line return formatting causing this?
There was a problem hiding this comment.
Rstudio has some formating options, removing trailing whitespace options, ensure empty line at the end of the file. For personal projects I usually stick with them, but all this unrelated noise make this pull request harder to read.
I'm not against the changes and from a quick glance they seem to improve the code, I just don't want to review all the lines of unrelated changes looking for possible issues in the new and changed code 😄
There was a problem hiding this comment.
Rstudio has some formating options, removing trailing whitespace options, ensure empty line at the end of the file. For personal projects I usually stick with them, but all this unrelated noise make this pull request harder to read.
Agreed. I feel like this isn't going to be an isolated thing. Temping now to make one big PR of just white-noise?
There was a problem hiding this comment.
First of, that's not my call to make (poking @ycphs) and ideally it would be a decision unrelated to an IDE configuration.
Personally, I would prefer to introduce such changes only when applying code changes. Iirc correctly, such whitespace related bulk commits make git bisecting and blaming harder, but I didn't check that.
Finally, I think that this should not be a blocker for this pull request, but I won't volunteer right now to review it. (And I know projects, where such pull request would be closed without any hesitation.)
There was a problem hiding this comment.
I totally agree with @JanMarvin. It is hard to read, but I will merge the PR. The next time it would be great if only the rows with actual changes is pushed.
R/writeData.R
Outdated
| @@ -14,6 +14,7 @@ | |||
| #' \code{c(startCol, startRow)}. | |||
| #' @param colNames If \code{TRUE}, column names of x are written. | |||
| #' @param rowNames If \code{TRUE}, data.frame row names of x are written. | |||
| #' @param row.names Deprecated, please use \code{rowNames} instead | |||
There was a problem hiding this comment.
I do not get this. This looks as if you added and deprecated it in the very same moment?
There was a problem hiding this comment.
row.names had been originally passed from write.xlsx() and converted to rowNames before writeData() and writeDataTable(). This was also done with col.names and colNames but I missed that because the latter wasn't used in testing.
There was a problem hiding this comment.
I understand. I initially was wondering, why you'd deprecate functions from a stable API, but using similar arguments looks like an improvement. 👍
R/writeDataTable.R
Outdated
| @@ -12,6 +12,7 @@ | |||
| #' A vector of the form c(startCol, startRow) | |||
| #' @param colNames If \code{TRUE}, column names of x are written. | |||
| #' @param rowNames If \code{TRUE}, row names of x are written. | |||
| #' @param row.names Deprecated, please use \code{rowNames} instead | |||
| @@ -383,91 +383,95 @@ addWorksheet <- function( | |||
| od <- getOption("OutDec") | |||
| options("OutDec" = ".") | |||
| on.exit(expr = options("OutDec" = od), add = TRUE) | |||
|
|
|||
There was a problem hiding this comment.
@JanMarvin some changes here
buildWorkbook() is intended to be used with an assignment operator, so an explicit return should reinforce this
adds warning and tests for use of col.names
Resolves #192 and #187.
Adds the
buildWorkbook()function to extract the relevant processes fromwrite.xlsx(). The method in which I used should also allow for all applicable parameters passed through...into the other function calls.I have also made some formatting changes and small improvements in some of the code I was looking at. A large portion is simplifying the use of
grep()andgrepl()and adding someassert_*()functions which should make parameter checking easier and reduce some of the duplicate code.It may be worth while, too, to update some of the tests that use
write.xlsx(x, file); wb <- read.xlsx(file)asbuildWorkbook()now accomplishes that.