Improve error message when plot.title.position is not correctly mentioned.

[olivroy]

Before this PR

ggplot() +theme(plot.title.position = "Null")
Error in `ggplot_gtable()`:
! `plot.title.position` should be either
  "\"panel\"" or "\"plot\"".

With this PR

ggplot() +theme(plot.title.position = "Null")
Error in `ggplot_gtable()` 
! `plot.title.position` should be either "panel" or "plot".

I wonder how we could better improve this message, so it becomes:

ggplot() +theme(plot.title.position = "Null")
Error in `theme()` 
! `plot.title.position` should be either "panel" or "plot".

But I don't know how to do that.

Edit: I added suggestion to use rlang::arg_match() to get rid of the if (), but this doesn't solve the error call.

[teunbrand]

Hi there,

Good catch on these errors that are formatted confusingly.
ggplot2 typically uses arg_match0() instead of the full arg_match(), due to argument checking often happening downstream somewhere.
You can use this function to create an error pointing to theme() as follows:

library(rlang)

title_pos <- "wrong input"
arg_match0(
  title_pos, c("panel", "plot"), 
  arg_nm = "plot.title.position", 
  error_call = expr(theme())
)
#> Error in `theme()`:
#> ! `plot.title.position` must be one of "panel" or "plot", not "wrong
#>   input".
#> Backtrace:
#>     ▆
#>  1. ├─rlang::arg_match0(...)
#>  2. └─rlang:::stop_arg_match(w, values = x, error_arg = y, error_call = z)
#>  3.   └─rlang::abort(msg, call = error_call, arg = error_arg)

^{Created on 2023-05-03 with reprex v2.0.2}

[olivroy]

Hi,

Thanks for the suggestion.

Would you like me to update my PR and snapshots with this instead?

# new
arg_match0(
  title_pos, c("panel", "plot"), 
  arg_nm = "plot.title.position", 
  error_call = expr(theme())
)
  # delete
 if (!(title_pos %in% c("panel", "plot"))) {
    cli::cli_abort("{.var plot.title.position} should be either {.val panel} or {.val plot}.")
}

[teunbrand]

That'd be great, thanks! plot.caption.position might also benefit from this treatment.

[olivroy]

Just a note,

globally, this part of the error message Error in ... is not tested in the snapshot tests.

 ggplot(mtcars) + geom_point(aes(disp, mpg)) + theme(plot.title.position = "test")
Error in `theme()`:
! `plot.title.position` must be one of "panel" or "plot", not
  "test".

It is the new error message. but I tried another thing for example with plot.background.

ggplot() + theme(plot.background = "w")
Error in `mapply()` 
! The `plot.background` theme element must be a <element_rect>
  object.

Edit: the mapply() part is not very informative! Clarify sentence.

[teunbrand]

Yes I think ggplot2 could do a little bit better in reporting where the error has originated sometimes. To be fair, ! The `plot.background` theme element must be a <element_rect> object. is a good error message, I think. The bad part is the Error in `mapply() bit. However, I think that a witch hunt for this ambiguous origin reporting is perhaps better for a separate PR, so I wouldn't worry too much about this for this PR. If you think this is important, you could open an issue about it so that we can discuss what the scope should be.

[teunbrand]

Looks good to me

[teunbrand]

I think the failing test on R devel is unrelated to this PR.

From the R news section:

density() more consistently computes grid values for the FFT based convolution, following Robert Schlicht's analysis and proposal in PR#18337, correcting density values typically by a factor of about 0.999. Optional old.coords=TRUE provides back compatibility.

Which causes geom_violin() (and presumably other density based stats) to change across versions. I don't know if there is a CI solution for this, but for now I think it's OK to merge this PR.

@olivroy Thanks for the work you've put into this PR!