Fix deparse round-trip for evaluated ggplot2 theme elements in parse_ggplot2_args()#345
Fix deparse round-trip for evaluated ggplot2 theme elements in parse_ggplot2_args()#345stefanlinner wants to merge 4 commits into
parse_ggplot2_args()#345Conversation
|
✅ All contributors have signed the CLA |
|
I have read the CLA Document and I hereby sign the CLA. |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
osenan
left a comment
osenan
left a comment
There was a problem hiding this comment.
Dear @stefanlinner
Thank you very much for your PR! I have not finished fully testing and reviewing your solution. But I can already suggest some changes:
- Style: Please do not use abbreviations for variable names, they make the code more confusing with time. I have highlighted one, review the text and rename variables to a more meaningful name that is easier to read for code maintainers.
- Tests: We try to test functions by calling only exported functions in the unit tests. Therefore, internal functions should be called indirectly, preparing scenarios and datasets that test all the code. Refactor the highlighted test so we only use the exported function.
Finally, please call spelling::update_wordlist with the words failing in the Spell Check job so it passes all CI checks.
| testthat::test_that("to_call passes through already-quoted calls unchanged", { | ||
| q <- quote(ggplot2::element_text(size = 20)) | ||
| testthat::expect_identical(teal.widgets:::to_call(q), q) | ||
| }) | ||
|
|
||
| testthat::test_that("to_call passes through atomic values unchanged", { | ||
| testthat::expect_identical(teal.widgets:::to_call("red"), "red") | ||
| testthat::expect_identical(teal.widgets:::to_call(42), 42) | ||
| testthat::expect_identical(teal.widgets:::to_call(TRUE), TRUE) | ||
| testthat::expect_null(teal.widgets:::to_call(NULL)) | ||
| }) | ||
|
|
||
| testthat::test_that("to_call reconstructs element_text", { | ||
| result <- teal.widgets:::to_call(ggplot2::element_text(size = 20)) | ||
| testthat::expect_true(is.call(result)) | ||
| # Verify round-trip | ||
| evaled <- eval(result) | ||
| testthat::expect_identical(evaled$size, 20) | ||
| }) | ||
|
|
||
| testthat::test_that("to_call reconstructs element_rect", { | ||
| result <- teal.widgets:::to_call(ggplot2::element_rect(fill = "blue", colour = "black")) | ||
| testthat::expect_true(is.call(result)) | ||
| evaled <- eval(result) | ||
| testthat::expect_identical(evaled$fill, "blue") | ||
| testthat::expect_identical(evaled$colour, "black") | ||
| }) | ||
|
|
||
| testthat::test_that("to_call reconstructs element_text with custom margin via round-trip", { | ||
| original <- ggplot2::element_text(size = 14, margin = ggplot2::margin(5, 10, 5, 10)) | ||
| result <- teal.widgets:::to_call(original) | ||
| testthat::expect_true(is.call(result)) | ||
| deparsed <- deparse(result) | ||
| evaled <- eval(parse(text = deparsed)) | ||
| testthat::expect_identical(evaled$size, 14) | ||
| testthat::expect_equal(as.numeric(evaled$margin), c(5, 10, 5, 10)) | ||
| }) | ||
|
|
||
| testthat::test_that("to_call reconstructs rel()", { | ||
| result <- teal.widgets:::to_call(ggplot2::rel(1.5)) | ||
| testthat::expect_true(is.call(result)) | ||
| testthat::expect_identical(eval(result), ggplot2::rel(1.5)) | ||
| }) | ||
|
|
||
| testthat::test_that("to_call warns for unrecognized non-atomic objects", { | ||
| # An arbitrary list that isn't a ggplot2 element | ||
| testthat::expect_warning( | ||
| teal.widgets:::to_call(structure(list(), class = "some_custom_thing")), | ||
| "could not be converted to a call" | ||
| ) |
There was a problem hiding this comment.
Hi, we do not include unit test directly calling the internal functions. Instead, please try calling parse_ggplot_args and prepare scenarios that indirectly will test to_call.
There was a problem hiding this comment.
Hi, thanks for your first feedback! I adapted the unit tests accordingly.
| #' @keywords internal | ||
| unit_to_call <- function(x) { | ||
| vals <- as.numeric(x) | ||
| u <- grid::unitType(x) |
There was a problem hiding this comment.
please do not use abbreviations
| u <- grid::unitType(x) | |
| unit_type <- grid::unitType(x) |
There was a problem hiding this comment.
Good point. I adapted the code accordingly.
| constructor <- utils::getFromNamespace(cls, "ggplot2") | ||
| default_obj <- tryCatch(constructor(), error = function(e) NULL) | ||
| # "color" is an alias for "colour" in ggplot2 | ||
| param_names <- setdiff(names(formals(constructor)), c("...", "inherit.blank", "color")) |
There was a problem hiding this comment.
| param_names <- setdiff(names(formals(constructor)), c("...", "inherit.blank", "color")) | |
| param_names <- setdiff(names(formals(constructor)), c("...", "color")) |
Not sure why we're excluding inherit.blank. The way it is now, if user set inherit.blank = TRUE, it will never go through.
There was a problem hiding this comment.
Good catch! I included that while testing and forgot to remove it later on. Thanks!
3 participants
Fixes insightsengineering/teal.modules.general#974
TrackB_RMedicine_Hackathon_Apr2026
Problem
When users pass evaluated ggplot2 theme elements to
ggplot2_args(), for example:the element objects get embedded as complex nested structures. When
parse_ggplot2_args()builds the call viaas.call()and teal's pipeline later deparses it to a string, the result looks like:This fails on re-parse because
structure()cannot faithfully reconstruct nested grid/unit internals. The workaround so far has been to requirequote()around element calls, which is not obvious to users.Solution
This PR adds a
to_call()helper (and supporting internal functionselement_to_call(),margin_to_call(),unit_to_call()) that converts already evaluated ggplot2 element objects back into their constructor call form. For instance, an evaluatedelement_text(size = 20)object is reconstructed as the callggplot2::element_text(size = 20).The conversion is applied in
parse_ggplot2_args()vialapply(ggplot2_args$theme, to_call)before the call is assembled. This is the last point where we control call construction before teal'seval_code()pipeline deparses the expression.Covered element types:
element_text,element_line,element_rect,element_blank,rel(),margin(),unit(), andwaiver(). Already quoted calls and plain atomic values pass through unchanged. Unrecognized objects trigger a warning that suggests usingquote()as a fallback.Trade-offs
The reconstruction relies on
utils::getFromNamespace()andformals()to introspect ggplot2 constructors at runtime, comparing property values against a default instance to determine which arguments the user actually set. This is coupled to ggplot2 internals, but the alternative (requiringquote()everywhere) is a worse user experience.Other changes
gridtoImportsin DESCRIPTION (needed forgrid::unitType()in margin/unit reconstruction).packageVersion("ggplot2") <= "3.5.2") from tests. These branches were testing the old broken deparse output; withto_call()the output is clean across versions.to_call()covering each element type, passthrough behavior, the nested margin case, and the warning for unrecognized objects.