Skip to content

Commit

Permalink
Polising magical defaults
Browse files Browse the repository at this point in the history
  • Loading branch information
hadley committed Jul 25, 2023
1 parent 5a87c5e commit 4b435ce
Showing 1 changed file with 20 additions and 60 deletions.
80 changes: 20 additions & 60 deletions def-magical.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
#| include = FALSE,
#| cache = FALSE
source("common.R")
source("fun_def.R")
```

```{r}
#| eval = FALSE,
#| include = FALSE
source("fun_def.R")
funs <- c(pkg_funs("base"), pkg_funs("stats"), pkg_funs("utils"))
funs %>% funs_formals_keep(~ is_symbol(.x) && !is_missing(.x))
pkg_funs("base") %>% funs_body_keep(has_call, "missing")
Expand All @@ -25,10 +25,13 @@ Magical defaults are best avoided because they make it harder to interpret the f
- In `data.frame()`, the default argument for `row.names` is `NULL`, but if you supply it directly you get a different result:

```{r}
fun_call(data.frame)
args(data.frame)
x <- setNames(nm = letters[1:2])
x
x <- setNames(nm = letters[1:3])
data.frame(x)
data.frame(x, row.names = NULL)
```
Expand All @@ -38,28 +41,11 @@ Magical defaults are best avoided because they make it harder to interpret the f
```{r}
#| error = TRUE,
#| fig.show = "hide"
fun_call(hist.default)
args(hist.default)
hist(1:10, xlim = c("Sturges", "Sturges"))
```
- In `Vectorize()`, the default argument for `vectorize.args` is `arg.names`, but this variable is defined inside of `Vectorize()`, so if you supply it explicitly you get an error.
```{r}
#| error = TRUE
fun_call(Vectorize)
Vectorize(rep.int, vectorize.args = arg.names)
```
- In `rbeta()`, the default value of `ncp` is 0, but if you explicitly supply it the function uses a different algorithm:
```{r}
rbeta
```
- In `table()`, the default value of `dnn` is `list.names(...)`; but `list.names()` is only defined inside of `table()`.
- `readr::read_csv()` has `progress = show_progress()`, but until version 1.3.1, `show_progress()` was not exported from the package.
That means if you attempted to run it yourself, you'd see an error message:
Expand All @@ -68,16 +54,14 @@ Magical defaults are best avoided because they make it harder to interpret the f
show_progress()
```
- In `usethis::use_rmarkdown_template()`, `template_dir` has the default value of `tolower(asciify(template_name))`, but `asciify` is not exported.
That means there's no way to interactively explore this default value.
## What are the exceptions?
It's ok to use this behaviour when you want the default value of one argument to be the same as another.
For example, take `rlang::set_names()`, which allows you to create a named vector from two inputs:
```{r}
fun_call(set_names)
library(rlang)
args(set_names)
set_names(1:3, letters[1:3])
```
Expand All @@ -95,20 +79,21 @@ If you use this technique, make sure that you never use the value of an argument
For example, in `file.copy()` `overwrite` defaults to the same value as `recursive`, but the `recursive` argument is defined after `overwrite`:

```{r}
fun_call(file.copy)
args(file.copy)
```

This makes the defaults arguments harder to understand because you can't just read from left-to-right.

## What causes the problem?

There are three primary causes:
This problem is generally easy to avoid for new functions:

- Overuse of lazy evaluation of default values, which are evaluated in the environment of the function, as described in [Advanced R](https://adv-r.hadley.nz/functions.html#default-arguments). Here's a simple example:
- Don't use default values that depend on variables defined inside the function.
The default values of function arguments are lazily evaluated in the environment of the function when they are first used, as described in [Advanced R](https://adv-r.hadley.nz/functions.html#default-arguments). Here's a simple example:

```{r}
f1 <- function(x = y) {
y <- trunc(Sys.time(), units = "months")
y <- 2
x
}
Expand All @@ -120,7 +105,7 @@ There are three primary causes:
When `x` takes the value `y` from its default, it's evaluated inside the function, yielding `1`.
When `y` is supplied explicitly, it is evaluated in the caller environment, yielding `2`.
- Use of `missing()` so that the default value is never consulted:
- Don't use `missing()`[^def-magical-1].
```{r}
f2 <- function(x = 1) {
Expand All @@ -135,42 +120,17 @@ There are three primary causes:
f2(1)
```
- In packages, it's easy to use a non-exported function without thinking about it.
- Don't use unexported functions.
In packages, it's easy to use a non-exported function without thinking about it.
This function is available to you, the package author, but not the user of the package, which makes it harder for them to understand how a package works.
## How do I remediate the problem?
This problem is generally easy to avoid for new functions:
- Don't use default values that depend on variables defined inside the function.
- Don't use `missing()`[^def-magical-1].
- Don't use unexported functions.
[^def-magical-1]: The only exceptions are described in @sec-args-mutually-exclusive and @sec-args-compound.
[^def-magical-1]: The only exceptions are described in @sec-args-mutually-exclusive) and @sec-args-compound).
## How do I remediate the problem?
If you have a made a mistake in an older function you can remediate it by using a `NULL` default, as described in @sec-def-short).
If the problem is caused by an unexported function, you can also choose to document and export it.
```{r}
`%||%` <- function(x, y) if (is.null(x)) y else x
f1_better <- function(x = NULL) {
y <- trunc(Sys.time(), units = "weeks")
x <- x %||% y
x
}
f2_better <- function(x = NULL) {
x <- x %||% 2
x
}
```

This modification should not break existing code, because expands the function interface: all previous code will continue to work, and the function will also work if the argument is passed `NULL` input (which probably didn't previously).
Remediating this problem shouldn't break existing code, because it expands the function interface: all previous code will continue to work, and the function will also work if the argument is passed `NULL` input (which probably didn't previously).
For functions like `data.frame()` where `NULL` is already a permissible value, you'll need to use a sentinel object, as described in @sec-args-default-sentinel.
Expand Down

0 comments on commit 4b435ce

Please sign in to comment.