unify function parameters #38
Comments
|
|
|
|
Should break this into groups, maybe identical to the one one pagedown reference site. It is expected for a set of groups to share some arguments (e.g. the xml functions share arguments no other function needs). library(openxlsx2)
funs <- ls("package:openxlsx2")
funs <- funs[!funs %in% c("style_mgr", "wbChartSheet", "wbComment", "wbHyperlink", "wbSheetData", "wbWorkbook", "wbWorksheet")]
arguments <- sapply(funs, function(x) names(formals(x)))
uniargs <- unique(unlist(arguments))
argmat <- as.data.frame(
matrix("", nrow = length(arguments), ncol = length(uniargs))
)
colnames(argmat) <- uniargs
rownames(argmat) <- names(arguments)
for (row in seq_along(arguments)) {
argmat[row, arguments[[row]]] <- "x"
} |
|
Hello, I don't know if it is related, but I think that argument names should stick to one convention: either snake_case or camelCase.. It will be hard for the package to take its place if it's hard to remember if arguments are snake case or not. |
|
Hi @olivroy , of course it is related, but as you can see, the initial issue is now one and half year old. Since then not a lot of time was spent on the issue. I'm planning to do a 1.0 release soon -- maybe the next release or the one after the next release -- which shall provide a stable API So any work regarding the API has to be introduced in the upcoming weeks. The tricky part is providing a backward compatible solution, one that does not break all existing code. But I'm not worried about its place in history. I assume that this package provides a unique solution to a variety of issues and it fulfills mine just fine, probably that's why I haven't already worked on this. But if you want to work on this, please reach out, maybe begin with a small pull request and I'm pretty sure that we can work something out. |
|
Yes. I agree. It is very interesting I may look into it! The lifecycle tidyverse package may help with this https://lifecycle.r-lib.org/articles/communicate.html#renaming-an-argument I assume you'd rather have arguments be camelCase and all functions to snake_case? Edit: functions rename
convertToExcelDate -> convert_to_excel_date
optional:
create_tablestyle -> create_table_style
_numfmt -> _num_fmt (since using numFmt in some places)
# maybe issues with the documentation here since R6 class are camelCase (maybe wb_sheet_data could have @keywords internal
wb_sheet_data Parameters |
|
I do not really see the lifecycle package required, the same effect can be created using Renaming functions is simpler than renaming arguments especially if one wants to avoid creating dozens of identical arguments. |
|
I agree that renaming functions is easier. But what do you think about renaming arguments to camelCase like font_name -> font, font_size -> fontSize etc. I could check this out in the coming weeks if you think it's useful! |
|
I'm not sure if this would be really helpful or if it would create more confusion to the user when camel case and snake case are mixed together. My approach would be to switch arguments to snake case too, hoping that most of the current functions are already snake case, but maybe provide a Something like this (we use a similar approach with x <- "camelCase"
# adaption of https://stackoverflow.com/a/43768828/12340029
gsub(
pattern = "(\\G(?!^)|\\b[a-zA-Z][a-z]*)([A-Z][a-z]*|\\d+)",
replacement = "\\L\\1_\\2",
x = x, perl = TRUE
)
#> [1] "camel_case" |
|
Ah ok! Well, it seems like a harder task to convert arguments to snake_case... from what I can see currently, most arguments are camel case. and most functions are snake case The only functions that have exclusively snake_case arguments seem to be But most have camelCase arguments. For reference, in ggplot2, all the functions are camel_case, but arguments are separated by dots. i.e. |
|
Thanks for the hint. I cannot say that I've given it to much thought lately, but if things are charged I would want to provide a consistent API and breakage should be minimal. Like under the hood changes and soft deprecating. But I fully agree that it's not a trivial change and one that requires some thought. Hence, the unreadable table above. And unfortunately while everyone loves a nice API and a well written documentation, there were always other more interesting things to do. |
|
I want to change the order where required so that |
@jmbarbone and @sjewo , do you guys have any recommendations how to move on? Snake case functions and camel case arguments or ...? My biggest wish is to avoid hard deprecations (if this is a word), especially since I've already written a couple of hundred lines of |
|
Came across |
|
For me, I think for a |
|
Appreciate the reply @jmbarbone . And as written before, I agree that if anything, switching to snake_case should be preferred. But I would want to avoid throwing around deprecation warnings on short notice. In a pre-1.0 release and deprecate in 1.0. Realistically, if we continue on a monthly release cycle, much of the current code using As an alternative, I suggest something like #678. While I know you weren't a fan of the under-the-hood changing, and probably still are, at least they guarantee that the old code still runs. The modified |
|
I think it is a very good idea to switch to snake_case. (I originally pushed for camelCase args, just because it seemed like the easiest, least effort) As I wanted to respect the existing code base, I didn't want to push it too much, but like I will take a look in the coming weeks, and see if / when I can contribute. |
|
I also think that a consistent use of spelling for arguments makes sense. I can't judge whether snake_case is more widely used than camelCase. Either way, if you want to change arguments without breaking user code, something like this procedure might be an option: |
|
Thanks @sjewo , at least in the tidyverse, googles style guide wants functions as |
|
I made a list of all the functions and arguments that are camelCase and should be converted to snake_case in #679 . And because that was such a boring task, I almost wanted to drop it, 20 functions in. Looking at this list, that's just too many changes all around, a gentle deprecation or dropping is off the table for me. Most of these functions are wrapper functions, so we have double the number of replacements in the R6 wb objects. This is a task comparable to counting sheep for me. |
|
To be less intrusive, the foo <- function(param_one = paramOne, param_two = paramTwo, ..., paramOne, paramTwo) {
force(param_one)
force(param_two)
list(param_one, param_two)
}
identical(
foo(paramOne = 1, paramTwo = 2),
foo(param_one = 1, param_two = 2)
)
#> [1] TRUE
cc_deprecate <- function(x, snake = NULL) {
camel <- deparse1(substitute(x))
if (!missing(x)) {
# convert cc camelCase to snake_case
if (is.null(snake)) {
snake <- gsub("([a-z])([A-Z])", "\\1_\\2", camel)
snake <- tolower(snake)
}
# get function that uses cc_deprecate
fun <- as.character(sys.call(-1))[1]
.Deprecated(msg = sprintf(
"Use '%s(%s)' instead of '%s(%s)'",
fun,
snake,
fun,
camel
))
}
invisible()
}
foo <- function(param_one = paramOne, param_two = paramTwo, ..., paramOne, paramTwo) {
force(param_one)
force(param_two)
cc_deprecate(paramOne)
cc_deprecate(paramTwo)
list(param_one, param_two)
}
foo(paramOne = 1, paramTwo = 2)
#> Warning: Use 'foo(param_one)' instead of 'foo(paramOne)'
#> Warning: Use 'foo(param_two)' instead of 'foo(paramTwo)'
#> [[1]]
#> [1] 1
#>
#> [[2]]
#> [1] 2
foo(param_one = 1, param_two = 2)
#> [[1]]
#> [1] 1
#>
#> [[2]]
#> [1] 2Created on 2023-07-09 with reprex v2.0.2 |
|
It is a smart approach and I like the I'm going with #' @param paramOne,param_one foo
#' @param paramTwo,param_two foo
#' @export
foo <- function(paramOne, param_one, paramTwo, param_two) {
...
}My approach is this #' @param param_one foo
#' @export
foo <- function(param_one = next_sheet(), ...) {
standardize(...) # convert everything in ... to snake_case including overrides
...
}
# should still work
foo(paramOne = next_sheet()))The only real issue I see with my approach is that |
|
This is and will remain an ongoing task for the foreseeable future, but the issue is now outdated |
currently there are a few varying options accross the functions. The worst are the counter intuitive ones in
cloneWorksheetThe text was updated successfully, but these errors were encountered: