Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better scale messages #5343

Merged
merged 21 commits into from
Sep 12, 2023
Merged

Better scale messages #5343

merged 21 commits into from
Sep 12, 2023

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #4185, fix #4258 and fix #1312.

Briefly, this PR sets the call field of a Scale ggproto class to a more informative call, and uses that call in the messaging.

Less brief: errors thrown in scales led to some confusion about which scale was throwing the error. Getting the appropriate call for every scale helps in throwing more informative messages. In addition, this PR deprecates the scale_name argument to scale constructors, as the call takes over its function. This is in opposition to suggested approach here: #1312 (comment). Some selected examples follow.

Previously, the below would throw:

Error: Continuous value supplied to discrete scale

Note that now it mentions the scale, and which values are unexpected.

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

# See #4258
ggplot() + 
  geom_point(aes(x = "A", y = 2)) +
  scale_x_continuous()
#> Error in `scale_x_continuous()`:
#> ! Discrete values supplied to continuous scale
#> ℹ Example values: "A"

Previously the following would throw the very awkward:

#> Warning in log(x, base): NaNs produced
#> Warning: Transformation introduced infinite values in discrete y-axis

Now it mentions the correct scale and the transformation.

# See #4185
ggplot() +
  geom_point(aes(1, 1, colour = -1)) +
  scale_colour_viridis_c(trans = "log10")
#> Warning in log(x, base): NaNs produced
#> Warning in scale_colour_viridis_c(trans = "log10"): 
#> log-10 transformation introduced infinite values.

I also passed down some calls to several checkers, which previously simply didn't mention the scale.

scale_alpha_continuous(breaks = 1:2, labels = "A")
#> Error in `scale_alpha_continuous()`:
#> ! `breaks` and `labels` must have the same length

Created on 2023-07-04 with reprex v2.0.2

Lastly, I also have an alternate reason for this PR. If we'd ever want to solve #4269, we'd need to know which parameters we're allowed to override. For example, if the user provides limits in a proper scale, but not breaks, we should be allowed to override the breaks (and maybe limits). In any case, we'd need to figure out what arguments were user supplied. This is now a lot easier:

scale <- scale_colour_discrete(breaks = c("A", "B", "C"), name = "My Scale")
call_args_names(scale$call)
#> [1] "breaks" "name"

@teunbrand teunbrand added scales 🐍 messages requests for improvements to error, warning, or feedback messages labels Jul 9, 2023
R/scale-colour.R Show resolved Hide resolved
R/scale-colour.R Show resolved Hide resolved
@thomasp85 thomasp85 self-requested a review August 29, 2023 18:41
Comment on lines -406 to +420
cli::cli_abort("Not implemented")
cli::cli_abort("Not implemented.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not hugely important but shouldn't these reference self$call as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These don't have self as an argument, so I was hesitant about this. Should I give them self as an argument?

Comment on lines -422 to +436
cli::cli_abort("Not implemented")
cli::cli_abort("Not implemented.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the few comments LGTM

@teunbrand
Copy link
Collaborator Author

I'm merging this despite Thomas' comments (to be fair, 'not hugely important' eerily looks like 'good enough').
The messages that currently lack a reported call are mostly developer-facing, and users should never see these.
Combined with a mild reluctance on my part to change the function signature: I'm happy with this as-is.

@teunbrand teunbrand merged commit 5da2d30 into tidyverse:main Sep 12, 2023
11 of 12 checks passed
@teunbrand teunbrand deleted the scale_messages branch September 12, 2023 13:19
@@ -105,12 +110,17 @@ scale_y_continuous <- function(name = waiver(), breaks = waiver(),
na.value = NA_real_, trans = "identity",
guide = waiver(), position = "left",
sec.axis = waiver()) {
call <- caller_call()
if (is.null(call) || !startsWith(as.character(call[[1]]), "scale_")) {
Copy link

@averissimo averissimo Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change created a problem when calling scale_y_continous within another package function via the double colon pkg::example_fun(...).

as.character(call[[1]]) contents is of length 3 and makes the if clause fails.

# [1] "::"         "tern"       "g_lineplot"

I created issue #5436

@teunbrand teunbrand mentioned this pull request Sep 28, 2023
cpsievert added a commit to plotly/plotly.R that referenced this pull request Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
messages requests for improvements to error, warning, or feedback messages scales 🐍
Projects
None yet
3 participants