[stefanlinner]

Fixes insightsengineering/teal.modules.general#974
TrackB_RMedicine_Hackathon_Apr2026

Problem

When users pass evaluated ggplot2 theme elements to ggplot2_args(), for example:

ggplot2_args(
  theme = list(text = element_text(size = 20))
)

the element objects get embedded as complex nested structures. When parse_ggplot2_args() builds the call via as.call() and teal's pipeline later deparses it to a string, the result looks like:

ggplot2::theme(text = structure(list(family = NULL, ..., margin = structure(...)), class = c("element_text", "element")))

This fails on re-parse because structure() cannot faithfully reconstruct nested grid/unit internals. The workaround so far has been to require quote() around element calls, which is not obvious to users.

Solution

This PR adds a to_call() helper (and supporting internal functions element_to_call(), margin_to_call(), unit_to_call()) that converts already evaluated ggplot2 element objects back into their constructor call form. For instance, an evaluated element_text(size = 20) object is reconstructed as the call ggplot2::element_text(size = 20).

The conversion is applied in parse_ggplot2_args() via lapply(ggplot2_args$theme, to_call) before the call is assembled. This is the last point where we control call construction before teal's eval_code() pipeline deparses the expression.

Covered element types: element_text, element_line, element_rect, element_blank, rel(), margin(), unit(), and waiver(). Already quoted calls and plain atomic values pass through unchanged. Unrecognized objects trigger a warning that suggests using quote() as a fallback.

Trade-offs

The reconstruction relies on utils::getFromNamespace() and formals() 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 (requiring quote() everywhere) is a worse user experience.

Other changes

• Added grid to Imports in DESCRIPTION (needed for grid::unitType() in margin/unit reconstruction).
• Removed ggplot2 version branching (packageVersion("ggplot2") <= "3.5.2") from tests. These branches were testing the old broken deparse output; with to_call() the output is clean across versions.
• Added unit tests for to_call() covering each element type, passthrough behavior, the nested margin case, and the warning for unrecognized objects.

[github-actions]

✅ All contributors have signed the CLA
_{Posted by the CLA Assistant Lite bot.}

[stefanlinner]

I have read the CLA Document and I hereby sign the CLA.

[stefanlinner]

recheck

[stefanlinner]

I have read the CLA Document and I hereby sign the CLA

[stefanlinner]

recheck

[osenan]

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.

[osenan]

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.

[stefanlinner]

Hi, thanks for your first feedback! I adapted the unit tests accordingly.

[osenan]

please do not use abbreviations

Suggested change

	u <- grid::unitType(x)
	unit_type <- grid::unitType(x)

[stefanlinner]

Good point. I adapted the code accordingly.

[donyunardi]

Suggested change

	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.

[stefanlinner]

Good catch! I included that while testing and forgot to remove it later on. Thanks!

[osenan]

Hi, thanks for your changes.
Instead of adding line by line comments I am taking your solution and suggesting a refactored version of the functions with a much simpler version.
Still, all your new tests are passed and also your good updates on the tests.

[osenan]

Suggested change

	theme_args <- lapply(ggplot2_args$theme, to_call)
	theme_args <- lapply(ggplot2_args$theme, theme_value_as_call)

Suggesting this change related with the next suggestion

[osenan]

Suggested change

	#' Convert an evaluated ggplot2 theme element to its call representation
	#'
	#' Ensures that evaluated element objects (e.g. from `element_text(size = 20)`)
	#' produce clean, deparseable calls instead of opaque `structure(...)` output
	#' that may fail to reconstruct correctly after a deparse/parse round-trip.
	#'
	#' Already-unevaluated calls and plain atomic values pass through unchanged.
	#'
	#' @param x An R object, typically a value from the `theme` list of a
	#' [ggplot2_args()] object.
	#' @return A call or atomic value suitable for embedding in an `as.call()`
	#' expression.
	#' @keywords internal
	to_call <- function(x) {
	if (is.null(x) || is.call(x) || is.name(x)) return(x)
	if (inherits(x, "element_blank")) return(quote(ggplot2::element_blank()))
	if (inherits(x, "waiver")) return(quote(ggplot2::waiver()))
	if (inherits(x, "rel")) {
	return(as.call(list(str2lang("ggplot2::rel"), as.numeric(x))))
	}
	# Check both bare and namespaced class (ggplot2 < 4.0 vs >= 4.0)
	if (inherits(x, "margin") || inherits(x, "ggplot2::margin")) return(margin_to_call(x))
	if (inherits(x, "unit")) return(unit_to_call(x))
	if (inherits(x, "element")) return(element_to_call(x))
	if (is.atomic(x)) return(x)
	warning(
	"Theme argument of class ", paste(class(x), collapse = "/"),
	" could not be converted to a call. ",
	"Wrap the value in quote() to ensure a clean deparse/parse round-trip.",
	call. = FALSE
	)
	x
	}
	#' @keywords internal
	element_to_call <- function(x) {
	element_class <- sub("^.*::", "", class(x)[1])
	fn_symbol <- str2lang(paste0("ggplot2::", element_class))
	constructor <- utils::getFromNamespace(element_class, "ggplot2")
	default_obj <- tryCatch(constructor(), error = function(e) NULL)
	# "color" is an alias for "colour" in ggplot2
	param_names <- setdiff(names(formals(constructor)), c("...", "color"))
	args <- list()
	for (param_name in param_names) {
	value <- tryCatch(x[[param_name]], error = function(e) NULL)
	if (is.null(value)) next
	if (!is.null(default_obj)) {
	default_value <- tryCatch(default_obj[[param_name]], error = function(e) NULL)
	if (identical(value, default_value)) next
	}
	args[[param_name]] <- to_call(value)
	}
	as.call(c(list(fn_symbol), args))
	}
	#' @keywords internal
	margin_to_call <- function(x) {
	values <- as.numeric(x)
	unit_type <- grid::unitType(x)[1]
	default_margin <- ggplot2::margin()
	if (identical(values, as.numeric(default_margin)) && identical(unit_type, grid::unitType(default_margin)[1])) {
	return(quote(ggplot2::margin()))
	}
	as.call(list(str2lang("ggplot2::margin"), values[1], values[2], values[3], values[4], unit_type))
	}
	#' @keywords internal
	unit_to_call <- function(x) {
	values <- as.numeric(x)
	unit_type <- grid::unitType(x)
	if (length(values) == 1L) {
	as.call(list(str2lang("grid::unit"), values, unit_type[1]))
	} else {
	as.call(list(str2lang("grid::unit"), values, unit_type))
	}
	}
	#' @noRd
	theme_value_as_call <- function(value) {
	if (!inherits(value, "S7_object")) {
	return(value)
	}
	if (inherits(value, "ggplot2::margin")) {
	return(as.call(c(
	list(quote(ggplot2::margin)),
	as.list(as.numeric(value)),
	list(unit = grid::unitType(value)[1])
	)))
	}
	constructor <- str2lang(class(value)[1])
	constructor_args <- names(formals(eval(constructor)))
	properties <- getExportedValue("S7", "props")(value)
	properties <- properties[intersect(names(properties), constructor_args)]
	properties <- Filter(Negate(is.null), lapply(properties, theme_value_as_call))
	as.call(c(list(constructor), properties))
	}

Hi, I think you find the root of the issue but the solution is too complex. It handles too many scenarios where we should return what was provided without modification. I am suggesting this function, which I have tried in the original repo and in your fork.
It works for all the tests except for the one where we expect a warning that was triggered in your function. I think this option is better and it will keep your contribution to the bug fixing.

[llrs-roche]

@stefanlinner sorry it took us quite a while to address this PR. Since then we haven't heard back from you we will implement a shorter approach to your proposed solution via a different PR. Many thanks for participating on the hackathon and contributing to teal.