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

Guard against old discrete scales with identity palettes #5933

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

teunbrand
Copy link
Collaborator

This PR is an amendement to #5770.

Briefly, we introduced the palette argument, but revdep check show that some people contstruct their discrete position scales using discrete_scale(..., palette = identity). The identity palette is problematic as it doesn't return the expected values. This PR replaces identity palettes with seq_len to keep consistent behaviour with older scales.

Comment on lines 143 to 146
# Guard against old use of having identity palettes
if (identical(.subset2(self, "palette"), identity)) {
self$palette <- seq_len
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we fix this during construction? instead of checking at every map call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants