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

Simplify scales$add_default #5409

Merged
merged 4 commits into from
Oct 2, 2023
Merged

Simplify scales$add_default #5409

merged 4 commits into from
Oct 2, 2023

Conversation

thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Sep 6, 2023

Fix #5408 and fix #4873

This PR fixes some corner case issues around the use of stage(). However, it also just cleans up the add_default() methods of Scales. Originally this method did a second evaluation of newly added aesthetics for the sake of deducing their type, but that information was already created in the calling function as part of the standard aesthetic evaluation. So, instead, the method is now passed in the evaled aesthetics rather than original data+aesthetics and saves a number of reevaluations

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

LGTM, but small comment you might like to consider.
Does this happen to also fix #4873?
CI on devel is failing (or probably will fail at time of writing) because {vdiffr} has trouble installing, but you might've been aware of that.

R/aes-evaluation.R Outdated Show resolved Hide resolved
@teunbrand
Copy link
Collaborator

Yes the one line looks much clearer now. Do you wish to merge this before new merge conflicts arise due to updates to the news file?

@thomasp85 thomasp85 merged commit 0e72d95 into main Oct 2, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants