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

Export is.waive() for extensions to use #6173

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

Conversation

arcresu
Copy link

@arcresu arcresu commented Nov 1, 2024

waiver() is an internal detail of ggplot that end-users don't really need to be aware of, but extensions that define new stat_*() or geom_*() functions generally need to be able to check for waivers for the same reason that ggplot itself does.

I find that most extensions I write need to copy the definition of is.waive(), and I'm not alone. Would you consider exporting it for extensions to use?

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

Agreed, this would be good to have available in extensions.
Would you mind adding a news bullet stating that is.waiver() is now exported?

@arcresu
Copy link
Author

arcresu commented Nov 3, 2024

Done. Though that does raise the question of whether this should be is.waive() or is.waiver(). I got used to the former, but since the class is waiver we could rename it? I'm happy either way.

@teunbrand
Copy link
Collaborator

Good point! I think is.waiver() makes more sense in this case then because it has symmetry between the class name and the test function. If you're feeling up for it, you can rename the function and replace all the is.waive() calls with is.waiver() calls, but if you rather not, I'm also happy to do it.

@arcresu
Copy link
Author

arcresu commented Nov 5, 2024

No problem; rename done.

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.

2 participants