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

Regressions related to #5879 #6008

Open
teunbrand opened this issue Jul 19, 2024 · 2 comments · May be fixed by #6078
Open

Regressions related to #5879 #6008

teunbrand opened this issue Jul 19, 2024 · 2 comments · May be fixed by #6078
Labels
bug an unexpected problem or unintended behavior

Comments

@teunbrand
Copy link
Collaborator

This is an issue meant to track fallout from #5879 as determined through reverse dependency check in dbea0a6.
Issue #6003 was already identified and fixed in #6004.

Another issue is that names(ggplot()) no longer contains the labels field. This trips up some tests in dependencies that expect that length(ggplot()) is some fixed value. This should be easy to fix.

Another issue is that people sometimes test expect_equal(plot$labels$var, "value"). As plot$labels is no longer filled upon adding layers, plot$labels is empty in many such cases and thus the tests fail. This affects >30 packages and is not straightforward to fix, so we'd need to decide wether to revert #5879 or ask maintainers to adapt their tests.

@olivroy
Copy link
Contributor

olivroy commented Jul 19, 2024

I think #5879 is a great addition! If you decide to keep this, I'd be happy to help submit PRs to other packages. Maybe helping moving the tests from tests that are quite sensitive and slow down ggplot2's process.

expect_length(ggplot(), <specific length>
# better test
expect_s3_class(ggplot(), "ggplot")

Maybe ggplot2 could benefit from having a get_aes_labels() which could return a named vector and be helpful for ggplot2 extensions. (Please disregard if it doesn't make sense).

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jul 19, 2024

Thanks for the offer @olivroy! The length(ggplot()) issue is very fixable, so we wouldn't need to bother other maintainers for this. I like the idea of writing a getter for the labels. For now let's wait a bit before sending out PRs, maybe some elegant solution comes to mind in the meanwhile.

@teunbrand teunbrand added the bug an unexpected problem or unintended behavior label Aug 5, 2024
@teunbrand teunbrand linked a pull request Sep 5, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants