Document breaking changes in internal API

[cpsievert]

At least a handful of pretty important packages (e.g., plotly, shiny, etc) rely on the data structure returned by ggplot_build()...it'd be really helpful to have a news note when these things change as well as a short statement on why they've changed.

Since the latest CRAN release (3.2.0), I've noticed at least one change, have there been any others?

[cpsievert]

It appears 6c2fe76 introduced another breaking change to [x/y].major (they are now NULL). I'm not sure if that is intentional or not...there are no unit tests on these fields

Before 6c2fe76:

library(ggplot2)
p <- ggplot(iris) +
  geom_point(aes(x = Petal.Width, y = Sepal.Length, color = Species)) +
   scale_x_continuous(breaks = seq(0,3,0.5), limits =c(0,3), labels = letters[1:7])
b <- ggplot_build(p)
b$layout$panel_params[[1]]$x.major

[1] 0.04545455 0.19696970
[3] 0.34848485 0.50000000
[5] 0.65151515 0.80303030
[7] 0.95454545

After 6c2fe76:

b$layout$panel_params[[1]]$x.major

NULL

[cpsievert]

Ahhh, it must be x$break_positions() now instead of x.major?

b$layout$panel_params[[1]]$x$break_positions()

[1] 0.04545455 0.19696970
[3] 0.34848485 0.50000000
[5] 0.65151515 0.80303030
[7] 0.95454545

[clauswilke]

I think this is all part of @paleolimbot's refactoring of axes. The end result should be much cleaner and more maintainable, but there's going to be some pain transitioning.

@paleolimbot It might be helpful if you could prepare a brief document explaining the key design choices and showing with simple examples how relevant information about the panels and axes can be extracted under your new system. This might fit into the (as of yet nonexistent) ggplot2 internal development vignette, or alternatively into the vignette on extending ggplot2.

[cpsievert]

but there's going to be some pain transitioning.

This pain could be significantly reduced if there was a "Breaking changes in internal API" section of the news that stated something like:

• Break values of positional scales have moved from [x/y].major to [x/y]$break_positions()
• Text labels of positional scales have moved from [x/y].labels to [x/y]$get_labels()

A vignette might be helpful in this particular case, but generally speaking, when these sort of changes are made, they should be documented in a simple and consistent fashion.

[paleolimbot]

@clauswilke is right, this is my fault! The overarching theme of my internship is formalizing the structure of the panel_params between coordinate systems, and changing how the guides are drawn. We haven't documented the changes yet because they're a few PRs away from done, but there will definitely be a prominent NEWS item and improved documentation for the internal Coord ggproto docs (and at least a blog post).

In the future panel$x and panel$y will be Scale-like objects, and panel$guides will be a list of guide objects (which will hopefully be ggproto in the near future) for the left/right/top/bottom of the panel. In the meantime, we could easily add back in the old syntax to the panel_params and deprecate it more slowly. The panel_params are entirely undocumented (and coord-specific) and I could only find references to x.range and y.range internally, which is why the breaks and labels were removed.

[cpsievert]

Now that I think of it, if possible, it'd be great to have informative warning (instead of NEWS item) if you try to access these deprecated fields.

[paleolimbot]

We could do that if we make panel_params ggproto and make those fields active bindings? I think it would be easier just to add the breaks back into panel_params and phase them out in a future release.

[paleolimbot]

Just a note that now that #3398 is merged, there are a few more changes in the CoordCartesian panel params (I don't think they break compatibility though). Between the previous changes implementing ViewScales and the most recent one (implementing the new axis guides), I think we will wait to see what problems come up, fix them, then document the changes.

[teunbrand]

This release we documented changes to the internals in its own header and I think this is probably something we should keep doing in the future.