Report failure when write.xlsx fails. #208
Conversation
This reverts commit 7e6a2b4.
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
=======================================
Coverage 65.60% 65.60%
=======================================
Files 34 34
Lines 8919 8920 +1
=======================================
+ Hits 5851 5852 +1
Misses 3068 3068
Continue to review full report at Codecov.
|
|
|
||
| ## delete temporary dir | ||
| unlink(dirname(xlsx_file), force = TRUE, recursive = TRUE) | ||
| if(returnValue == FALSE){ | ||
| if (returnValue == FALSE) { |
There was a problem hiding this comment.
@ycphs
Do you know if there any reason why we wouldn't want or be able to simply return the result of the file.copy()? Does using invisible(1) satisfy anything?
There was a problem hiding this comment.
Initially there was no return value and the feature request to have an optional made it necessary to have a default with no return value.
There was a problem hiding this comment.
The default value returned was invisible(1) which I don't see as useful as having the return value be the result of file.copy() instead -- especially since if file.copy() fails saveWorkbook() essentially fails. So I guess I'm asking: should invisible(1) ever be returned or should the result of just be file.copy() (invisibly) returned?
There was a problem hiding this comment.
yes that would make more sense and it helps to actually and leads to the initial error.
Fixes #190 .
The pre-commit hooks were set up to require styler, so I ran styler on the files i changed, but committed the style pieces separately in case you want to strip them out.
Added a testthat test to show the bug and re-ran all tests to confirm that the fix doesn't break anything else.