bluefoxr / COINr

COINr
https://bluefoxr.github.io/COINr/
Other
25 stars 8 forks source link

Suggestion to use snake case for all function names, function arguments as well as elements of coins #17

Closed paulrougieux closed 2 years ago

paulrougieux commented 2 years ago

The Changes from COINr v1.0.0 specifies that:

all “building” functions start with a capital letter. Plot functions start with plot. Analysis functions mostly start with get. Functions are generally in lower case.

Why not keep all function names in snake case?

The tidyverse style guide specifies that:

Variable and function names should use only lowercase letters, numbers, and . Use underscores () (so called snake case) to separate words within a name.

You could for example start the build functions with build_ ?

Looking at an example from the visualisation vignette:

plot_corr(coin, dset = "Normalised", iCodes = list("Physical"), Levels = 1)

The function arguments would be simpler to remember if they were in snake case, i.e icodes and levels.

Similarly the elements of a coin could be in lowercase as well :

library(COINr)
coin <- build_example_coin(quietly = TRUE)
names(coin)
 "Log"      "Meta"     "Data"     "Analysis"

Renaming functions and arguments to snake case is a matter of taste and not essential for the JOSS review.

paulrougieux commented 2 years ago

For example the elements of this vector are inconsistent, the first is in snake case, while the others have a capital letter:

https://github.com/bluefoxr/COINr/blob/49faac222d7c86db84b6ae7356a61a0d98b27ed2/R/examples.R#L32

bluefoxr commented 2 years ago

@paulrougieux I actually agree with you on this. However I have to think of the users of the package - I recently did a major update which aligned a lot of the function syntax, although it could still be improved as you point out. The problem is that renaming functions and function arguments causes a lot of problems in terms of backward compatibility and continuity for users and I would strongly prefer not to do that again (at least not in the short term).

Overall I'd say that the gain from further improving the syntax is not enough to justify the pain it would inflict on users. So I would request to keep names as they are for now and potentially make gradual changes that have minimal impact (e.g. renaming when a function is deprecated for another reason, etc, and keeping new functions to strict style guidelines). What do you think?

paulrougieux commented 2 years ago

"the gain from further improving the syntax is not enough to justify the pain it would inflict on users."

Backward compatibility for the users is a good argument. You can close this issue for now.

paulrougieux commented 2 years ago

For backward compatibility, you could rename function arguments without affecting the user for example with a function definition of the type:

plot_corr(coin, dset = "Normalised", icodes = list("Physical"), levels = 1, iCodes=icodes, Levels=levels)

A strategy suggested in this SO answer to the question How to deprecate an argument?

Example argument deprecation from Stack Overflow ```R foo <- function(x, y, new_arg = c("a", "b", "c"), old_arg = c("a", "b", "c")){ if (!missing("old_arg")){ warning("Argument deprecated, use new_arg instead. The parameter new_arg is set equal the parameter old_arg.") new_arg <- old_arg } new_arg <- match.arg(new_arg, c("a", "b", "c")) if(new_arg == "a"){ return(x*y) }else if(new_arg == "b"){ return(x+y) }else if(new_arg == "c"){ return(x-y) } } ```