search

tidyverse / ggplot2

An implementation of the Grammar of Graphics in R
https://ggplot2.tidyverse.org
Other
6.46k stars 2.02k forks source link

Regressions related to #5879 #6008

Open teunbrand opened 1 month ago

teunbrand commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.