-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Better scale messages #5343
Conversation
cli::cli_abort("Not implemented") | ||
cli::cli_abort("Not implemented.") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
cli::cli_abort("Not implemented") | ||
cli::cli_abort("Not implemented.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this 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
I'm merging this despite Thomas' comments (to be fair, 'not hugely important' eerily looks like 'good enough'). |
@@ -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_")) { |
There was a problem hiding this comment.
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
This PR aims to fix #4185, fix #4258 and fix #1312.
Briefly, this PR sets the
call
field of aScale
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 thecall
takes over its function. This is in opposition to suggested approach here: #1312 (comment). Some selected examples follow.Previously, the below would throw:
Note that now it mentions the scale, and which values are unexpected.
Previously the following would throw the very awkward:
Now it mentions the correct scale and the transformation.
I also passed down some calls to several checkers, which previously simply didn't mention the scale.
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 notbreaks
, we should be allowed to override thebreaks
(and maybelimits
). In any case, we'd need to figure out what arguments were user supplied. This is now a lot easier: