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

make sure there are no unknown named arguments fix #140 #141

Closed
wants to merge 1 commit into from

Conversation

daroczig
Copy link
Owner

No description provided.

#' @keywords internal
assert_no_unknown_args <- function() {
call <- match.call(definition = sys.function(1), call = sys.call(1), expand.dots = FALSE)
stopifnot("Unknown named argument provided." = is.null(names(call$`...`)))
Copy link
Contributor

@MichaelChirico MichaelChirico Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better to supply the argument(s)?

Suggested change
stopifnot("Unknown named argument provided." = is.null(names(call$`...`)))
if (!is.null(unknown_names <- names(call$`...`))) {
stop("Unknown argument(s) provided: ", toString(unknown_names))
}

@daroczig
Copy link
Owner Author

After taking a look at the test results, and thinking this through more, I think failing on named args is not a viable solution, as there are many valid cases to pass named args, e.g. when using JSON loggers, or the below dummy example:

log_info('Hello, {v}!', v = 'world')
INFO [2024-02-29 00:26:30] Hello, world!

@daroczig daroczig closed this Feb 28, 2024
@MichaelChirico
Copy link
Contributor

hmm, in that case, maybe it's the description of '...' that's misleading?

#' @param ... R objects that can be converted to a character vector via the active message formatter function

@daroczig
Copy link
Owner Author

I'm not sure if that's misleading, but suggestions welcomed. Formatters take the args and convert to string. Some formatters support named args .. but those are still R objects. If it's confusing, pls suggest better language 🙏

@MichaelChirico
Copy link
Contributor

MichaelChirico commented Feb 29, 2024

My initial read is that something like do.call(paste, within(lapply(list(...), as.character), sep = "") would be done. Indeed that appears the case for un-named entries to .... But named arguments are handled differently -- they're used as an environment for substituting values into the unnamed parts of ... (much like glue()). So this distinction of named/unnamed parts of ... is totally absent.

This is an instructive example I think:

log_info('{a}', '{a}{b}', '{a}{b}{c}', a=1, b=2, c=3)
# INFO [2024-02-28 16:21:37] 112123

@daroczig daroczig deleted the assert_no_unknown_args branch August 7, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants