Skip to content

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Dec 4, 2024

This PR aims to fix #6064 and fix #4910 and fix #6289 and fix #5843 and replaces #6112.

Briefly, it attempts to cast non-null palette arguments with as_discrete_pal()/as_continuous_pal(). These are currently shims.

This PR replaces the older one because I could implemented more cleanly now that #5946 is merged.

@teunbrand
Copy link
Collaborator Author

Includes fix for #6289, since it'd touch the same code.

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.

I know the proposed changes don't practically affect the evaluation but I prefer to collect logical tests that define a specific condition (if that makes sense)

R/scale-colour.R Outdated

has_old_args <- any(names(enexprs(...)) %in% c("h", "c", "l", "h.start", "direction"))

if (has_old_args || !is.null(type) && is.null(palette)) {
Copy link
Member

Choose a reason for hiding this comment

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

I know I'm being pedantic here but please make the same changes to the logic tests here and below :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah my bad, I should've spotted these!

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.

LGTM

@teunbrand teunbrand merged commit 2478406 into tidyverse:main Mar 25, 2025
12 of 13 checks passed
@teunbrand teunbrand deleted the flexible_palettes branch March 25, 2025 14:43
@teunbrand teunbrand mentioned this pull request Apr 25, 2025
5 tasks
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