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

Rename trans arguments to transform #5566

Merged
merged 25 commits into from
Dec 14, 2023

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #5558.

Briefly, we follow in {scales}'s footsteps by renaming the trans argument to transform.
This mostly affects scale functions and the secondary axis functionality.

Comment on lines +107 to +110
if (lifecycle::is_present(trans)) {
deprecate_soft0("3.5.0", "continuous_scale(trans)", "continuous_scale(transform)")
transform <- trans
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the crux of the PR. If trans is used, we assume they meant transform instead.

olivroy

This comment was marked as resolved.

@teunbrand
Copy link
Collaborator Author

Yes good call, thanks for catching that!

@thomasp85 thomasp85 added this to the ggplot2 3.5.0 milestone Dec 5, 2023
@teunbrand

This comment was marked as resolved.

@teunbrand
Copy link
Collaborator Author

I'm still somewhat worried that renaming the trans ggproto field will have a non-trivial impact on scale extensions.
Not all scale extensions run through the continuous/binned/discrete_scale() constructors that would populate the transformer field. An example what I could see going wrong in extensions are cases such as this:

https://github.com/tidyverts/tsibble/blob/04b36e7ee9ed4c7bded6b4b938275a0f23da6bd0/R/scales.R#L52-L60

@thomasp85
Copy link
Member

yeah, that was kind of the cases we were worried about - let's keep the field name as is

@thomasp85
Copy link
Member

or, we could do both an do a soft-deprecation inside the transform() method where it checks whether trans is NULL

R/scale-.R Outdated Show resolved Hide resolved
R/axis-secondary.R Outdated Show resolved Hide resolved
@thomasp85
Copy link
Member

Just to be sure, have you tested this branch against a scale extension that uses trans?

@teunbrand
Copy link
Collaborator Author

teunbrand commented Dec 13, 2023

Yeah I tested it against an extension and trans gets funneled to transformation when used with a proper constructor. I don't think the {tsibble} example I linked to earlier works as it should. Because scale_x_date() is used to construct the ggproto object that is inherited from, the transformation field is populated and transformation %||% trans thus returns the transformation field.

@teunbrand
Copy link
Collaborator Author

Should now be better backwards compatible:

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

ggplot(economics[1:20,], aes(date, unemploy)) +
  geom_line() +
  tsibble::scale_x_yearquarter()
#> Warning: Scale$trans was deprecated in ggplot2 3.5.0.
#> ℹ Please use Scale$transformation instead.
#> This warning is displayed once every 8 hours.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.

Created on 2023-12-14 with reprex v2.0.2

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
Copy link
Collaborator Author

Thanks for the review Thomas!

@teunbrand teunbrand merged commit 000a939 into tidyverse:main Dec 14, 2023
12 checks passed
@teunbrand teunbrand deleted the trans_to_transform branch December 14, 2023 09:14
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.

Should the trans argument in scale_*_continuous etc be renamed to transform?
3 participants