-
Notifications
You must be signed in to change notification settings - Fork 2k
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
#4173 lambda functions in discrete scales & facets #4188
Conversation
In the future, please don't open a new PR just because something needs correcting in the previous PR. You can just push additional commits and the PR will update. |
@netique We recently merged in another PR that touched upon some of the same areas - can I get you to update this PR so it incorporates the changes in the master branch? |
scale part was resolved by tidyverse#4427
Sure, done. |
R/scale-.r
Outdated
@@ -184,6 +184,7 @@ discrete_scale <- function(aesthetics, scale_name, palette, name = waiver(), | |||
|
|||
check_breaks_labels(breaks, labels) | |||
|
|||
if (is.formula(labels)) labels <- as_function(labels) |
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.
Sorry this is mostly my fault for getting in your way with #4427, but the allow_lambda()
statements below already capture formulas as functions, so this line has become unnecessary. Apologies!
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.
That's OK, thank you for your contribution. In the up-to-date commit, the line is removed. I accidentally retained both solutions during the merge conflict resolution.
R/labeller.r
Outdated
|
||
dots <- list(...) | ||
|
||
|
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.
Can I get you to revert these superfluous changed
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.
My apologies. Fixed in 6d488b6.
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.
No apology needed - thanks for your work on this
Last thing - can I get you to add a news bullet about the change |
Thanks! |
Fix #4173
second PR, previous one was failing tests... now it should work as intended