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

ScalesList methods #5144

Merged
merged 4 commits into from
Apr 22, 2023
Merged

ScalesList methods #5144

merged 4 commits into from
Apr 22, 2023

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Jan 10, 2023

This PR aims to fix #1310.

In brief, ye olde issue suggested to convert the scales_*() functions to ggproto methods of the ScalesList object, which is what this PR does.

I think it has 2 benefits:

  • The ScalesList$scales don't need to be passed around, making such calls a little bit more terse.
  • For consistency purposes this is nice, as the upcoming ggproto'isation of the guides follows a similar pattern.

@thomasp85
Copy link
Member

blast from the past :-)

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 - can you confirm that no implementations have been changed (the diff is pretty messed up so I don't fully trust my eye)

@teunbrand
Copy link
Collaborator Author

teunbrand commented Mar 22, 2023

Other than some style things, the only thing I changed is has_default_transform() check that was in the $transform_df() method and applied it to the $backtransform_df() method as well.

I don't know if it is needed, but it would make the two methods more parallel. (as a reminder, you added this in 101c3a6). Happy to revert if you didn't apply it to de backtransform for good reasons.

The diff is horrible, I'd just open the 1st and 3rd commit side-by-side.

@thomasp85
Copy link
Member

that was most likely an oversight by me... I also didn't add any unit tests for it by the looks of it - could I get you to add such a test to make sure we don't end there again

@teunbrand
Copy link
Collaborator Author

I'm not quite sure what prompted the has_default_transform() check. I've guessed it would be custom trans_new() or Scale$transform(), but I can't trigger the code to fail if I leave out the check, so I'm unsure what to test for.

@teunbrand
Copy link
Collaborator Author

I'm merging this. If we want to add a test for the has_default_transform() logic, could you open up a new issue with a little bit more information?

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.

ScalesList-related functions should be moved into ggproto object
2 participants