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

Miscellaneous improvements to guides #5345

Merged
merged 16 commits into from
Aug 7, 2023

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix a series of backward compatibility issues that the GuideOld class didn't (perfectly) solve, in addition to a few polishes. It originates from a reverse dependency check which revealed what issues packages have with the guide system. This exercise has surfaced some problems that are being addressed in this PR. The main issues I have found are:

  • The S3 guide extensions that relied upon S3 NextMethod() class inheritance. Since we've gutted these in ggplot2, we had anticipated that these needed to be reimplemented in the package extensions themselves. This is mostly handled in PRs towards these packages.
  • We may have been too strict in letting guides() throw an error with empty or unnamed input. Some of the packages relied on guides() == NULL, which is now the case, whereas others provided unnamed guides, which is now a warning instead of an error.
  • Every so often a unit test assumed that g <- guides(colour = "colourbar") would store the information in g$colour, whereas this information is now stored in g$guides$colour and variations of this. This also has been addressed in PRs to those packages.

All in all, surveying this has led to the following PRs that are ready for review:

And PRs that rely on fixes in this PR to become ready for review:

Unrelated to this PR: besides issues with the guide system, it was also common to see the expect_equal(length(plot), 9) in tests, which should now be 10, or the direct borrowing of internals that we've superseded.

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

@teunbrand teunbrand merged commit 466344a into tidyverse:main Aug 7, 2023
11 of 12 checks passed
@teunbrand teunbrand deleted the guide_improvements branch August 7, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants