-
Notifications
You must be signed in to change notification settings - Fork 11
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
Deprecate old functions write_*()
and remove_*()
#1064
Conversation
…rite_data()`, `write_datatable()`
Thanks! Looking good already, but it’s 1pm here and I’m going to sleep now. Nitpicking: Please move the old functions below the do functions. Just so the code can be read from top. |
moved all old functions to own file. wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, left some minor remarks.
R/deprecate.R
Outdated
#' @keywords internal | ||
#' @export | ||
convertToExcelDate <- function(df, date1904 = FALSE) { | ||
.Deprecated(old = "convertToExcelDate", new = "convert_to_excel_date", package = "openxlsx2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess we can remove this line? The code will stop on the next either way. Should we still export this? I would simply no longer export this and drop the function after a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we stop exporting. People using it will get function not found, which is basically equivalent to dropping it fully (which I don't mind doing) I couldn't find any usage on GitHub. But no. Let's keep it like this.
Otherwise, people will not get the nudge to use convert_to_excel_date()
remove_comment()
write_comment()
, write_formula()
, `w…write_*()
and remove_*()
Thank you! 🎉 |
I just ran the revdep check workflow just to make sure it is fine (I didn't see anything, but may still want to make sure!) |
Good thinking! |
Just for a follow up: we might want to either add some tests for the |
You are right. or add the file to |
I have no doubts, it just looks a little red in the coverage chart 😄 |
remove_comment()
write_comment()
,write_formula()
,write_datatable()
,convertToExcelDate()
->convert_to_excel_date()
#1060