Add overwrite support to writeData for clean rewrite workflows#536
Add overwrite support to writeData for clean rewrite workflows#536Rong-Zh wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in overwrite flag to writeData() to support “clean rewrite” workflows by clearing existing worksheet cell data before writing new data, preventing stale trailing rows/cells when the new dataset is smaller.
Changes:
- Add
overwrite = FALSEparameter towriteData()and validate it. - When
overwrite = TRUE, delete existing worksheetsheet_databefore writing. - Update generated Rd documentation to include the new parameter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| R/writeData.R | Adds overwrite parameter and implements pre-write clearing of existing worksheet cell data. |
| man/writeData.Rd | Documents the new overwrite parameter in the public API docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Rong-Zh
left a comment
There was a problem hiding this comment.
This change fixes a potential data-loss issue when overwrite is TRUE.
The overwrite deletion now happens after table-header validation, so if validation fails or the function exits early, existing worksheet content is preserved.
|
Hi @Rong-Zh , thanks for the pull request! To be honest, I'm a bit indifferent about this. I guess everyone who needed this, should have some workaround by now? But the change is minimal enough for me to not bother. Just asking: does Also please note, |
|
No need to bother with the lintr warnings. Probably the return_lintr should be disabled, it is a bit of a nuisance. |
Thanks very much for the feedback. We currently depend on openxlsx in our production workflows. While implementing a local workaround, I made a very small adjustment and thought it might be helpful to contribute it back as a minor improvement. I fully understand your point that the package is no longer actively maintained, so I tried to keep the change as minimal and non-intrusive as possible. If you feel it is better not to include it, I completely understand and am happy to follow your preference. |
|
Ah well, you already did the work. Could you add a test for this and add a note in the NEWS file? |
I introduced
overwrite = TRUEinwriteData()to fix a common rewrite issue.In a typical workflow, users read an existing worksheet, filter rows, and write the filtered result back to the same sheet.
When the filtered result has fewer rows than the original data, the previous behavior could leave stale trailing rows in the worksheet.
Before this change:
writeData()only overwrote the current target write range.deleteData()and calculate row/column ranges,removeWorksheet()+addWorksheet()could also change the original sheet order.This was error-prone and easy to misuse.
After this change:
writeData(..., overwrite = TRUE)clears existing worksheet cell data before writing.overwrite = FALSE), so existing code stays backward compatible.In short, this change improves correctness for filtered rewrite scenarios while keeping the original API behavior intact for all existing callers.
Reproducible Setup: Generate Test Excel File
Explanation (without overwrite):
dfhas 4 rows.writeData(wb, sheet = name, df)writes only the new target range.deleteData()) before writing.Explanation (with overwrite):
writeData(wb, sheet = name, df, overwrite=TRUE)clears existing worksheet cell data before writing.deleteData()range calculation, and no need to remove/recreate the worksheet.