Nicer error message in coord-cartesian

[92amartins]

This is an attempt to fix https://github.com/tidyverse/ggplot2/issues/4601

[92amartins]

Thanks. Can I get you to update to match the style guide (suggested change), as well as add a unit test for the two errors

Thanks, for the review @thomasp85!

I applied your suggestion regarding style in f047b04a0a99adbb00eb47d803d87e4e0a856f43

And wrote a unit test for the arguments in eec525e380f3a2a39c65662307afb44b7eb3b132

Please let me know if further improvements are needed.

[teunbrand]

I think this is a good check, but shouldn't this check -in theory- be done for all coords that support the xlim/ylim arguments?

[92amartins]

Good point, @teunbrand! I'll extend the checks for other functions in the coord family.

[teunbrand]

I've been thinking about this again, and I think it shouldn't just test for is.environment(). Instead, I think that the limits should satisfy is.vector(limit) && length(limit) == 2, but this makes the error message slightly more complicated to be informative.

[92amartins]

I tried to implement the check as follows:

bad_xlim = !(is.vector(xlim) && length(xlim) == 2)
if (bad_xlim) {
    cli::cli_abort("...")
}

But, neither of those checks seem to work inside the function.

bad_xlim evaluates to FALSE even when I provide xlim=c(2,3).

@teunbrand can you see a better way to implement this check?

[teunbrand]

If you intend to recycle the checks in other coord_*() functions, it might be worth it to write a check function for coord limits. Here is a rough draft for one:

library(rlang)

check_coord_limits <- function(
  limits, arg = caller_arg(limits), call = caller_env()
) {
  if (is.null(limits)) {
    return(invisible(NULL))
  }
  if (!is.vector(limits) || length(limits) != 2) {
    what <- "{.obj_type_friendly {limits}}"
    if (is.vector(limits)) {
      what <- paste0(what, " of length {length(limits)}")
    }
    cli::cli_abort(
      paste0("{.arg {arg}} must be a vector of length 2, not ", what, "."),
      call = call
    )
  }
  invisible()
}

# Erroneous input
xlim <- ggplot2::xlim(0, 2)
ylim <- 1:3

check_coord_limits(xlim)
#> Error:
#> ! `xlim` must be a vector of length 2, not a
#>   <ScaleContinuousPosition/ScaleContinuous/Scale/ggproto/gg> object.
#> Backtrace:
#>     ▆
#>  1. └─global check_coord_limits(xlim)
#>  2.   └─cli::cli_abort(...)
#>  3.     └─rlang::abort(...)
check_coord_limits(ylim)
#> Error:
#> ! `ylim` must be a vector of length 2, not an integer vector of length
#>   3.
#> Backtrace:
#>     ▆
#>  1. └─global check_coord_limits(ylim)
#>  2.   └─cli::cli_abort(...)
#>  3.     └─rlang::abort(...)

^{Created on 2023-04-17 with reprex v2.0.2}

[teunbrand]

As pointed out in #5297, the lengthy check I proposed earlier might be better replaced by vec_check_size(xlim, 2).

[92amartins]

@teunbrand Thanks for the guidance.

I updated the PR with a implementation based on your draft.

Here are some considerations:

• I used obj_is_vector instead of is.vector to address the concern about custom data types mentioned here.
• Decided not to use vec_check_size because it does not return a logical value which makes hard to provide useful error messages
• Applied the check to almost all the coord-* functions except coord_munch and coord_polar

Let me know if further improvements are needed.

[teunbrand]

Pinging @thomasp85 to approve the changes he requested

[teunbrand]

Thanks so much @92amartins for incorporating the commentary and for the PR!