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

Transformation suffixes to prefixes (attempt 2) #410

Merged
merged 9 commits into from
Nov 7, 2023

Conversation

teunbrand
Copy link
Contributor

This PR aims to fix #390. It supplants #397.

It does the following:

  • It renames all *_trans suffixed functions to transform_* prefixed functions. The suffixed versions are kept as an alias.
  • Changed the class of "trans" objects to "transform" class objects.
  • as.transform(), previously as.trans() has mechanism to look for the prefixed names first, then tries to find suffixed versions.

I was unsure about if trans_new() or trans_range() should be renamed. As they aren't transformations, I didn't rename them. I'd be happy to rename them if so desired.

@thomasp85
Copy link
Member

Let's rename it all :-) but keep the old around

@teunbrand
Copy link
Contributor Author

Right, but if we call it transform_new() and transform_range(), folks might expect that these are also transformations to be passed to scales. How do you feel about new_transform() and range_transform()?

Merge branch 'main' into transform_alias

# Conflicts:
#	NEWS.md
#	R/scale-continuous.R
#	man/cscale.Rd
#	man/pal_shape.Rd
@thomasp85
Copy link
Member

trans_new() should become new_transform() so it is in line with how constructors are generally named now

trans_range() should probably be renamed to something like trim_to_domain() or something

@teunbrand
Copy link
Contributor Author

Alright, should be OK now

@thomasp85 thomasp85 merged commit 1ed7b9a into r-lib:main Nov 7, 2023
12 checks passed
@thomasp85
Copy link
Member

Thanks!

@teunbrand teunbrand deleted the transform_alias branch November 7, 2023 11:40
@k-motwani
Copy link

Let's rename it all :-) but keep the old around

It seems like the old were not kept around which causes issues downstream. Could this be remedied?

@teunbrand
Copy link
Contributor Author

Could you be specific about which functions are missing?

@k-motwani
Copy link

Sorry, should have been more specific! I'm having an issue with backwards compatibility for 'trans' in previously functional scripts

@teunbrand
Copy link
Contributor Author

Do you have a minimal reproducible example of what previously worked fine and is giving issues now?

@k-motwani
Copy link

I'll get back to you on this. Thanks for the prompt attention!

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.

*_trans suffixed functions would be easier to use if they were renamed to be trans_* prefixed functions
3 participants