Accept default= in modify_defaults #1339
Conversation
R/with.R
Outdated
| warning( | ||
| "'default' is not an argument to linters_with_defaults(). Did you mean 'defaults'? ", | ||
| "This warning will be removed when with_defaults() is fully deprecated." | ||
| ) | ||
| defaults <- vals$default | ||
| vals$default <- NULL | ||
| valid_idx <- nms != "default" | ||
| nms <- nms[valid_idx] |
There was a problem hiding this comment.
Is there a way to use Recall() after the repair to keep the change localized?
Also shouldn't do this if there is also a defaults argument.
There was a problem hiding this comment.
Didn't we eliminate Recall() as viable for the other similar checks in lint()/lint_package() because of headaches with ...?
Also shouldn't do this if there is also a defaults argument.
OK, actually that will make things a lot easier
There was a problem hiding this comment.
oh wait, I'm not sure that's possible inside modify_defaults. Maybe with running identical(defaults, default_linters). Otherwise need to do the backport inside linters_with_defaults. seems like overkill for something I can't really imagine happening.
There was a problem hiding this comment.
It would definitely be cleaner to put the warning in linters_with_defaults() though...
Shouldn't something like this work?
dots <- list(...)
if (missing(defaults) && "default" %in% names2(dots)) {
warning("...")
names(dots)[names(dots) == "default"] <- "defaults"
return(do.call(modify_defaults, dots))
}There was a problem hiding this comment.
that was my first try, but it breaks the substitute(alist(...)) logic in modify_defaults
There was a problem hiding this comment.
In what way? Maybe we can also use substitute(alist(...)) here to generate the correct call?
Possibly even modifying match.call() instead. I'd be fine if the call delegated to a correct invocation of linters_with_defaults() before jumping into modify_defaults().
There was a problem hiding this comment.
OK, seems working now after pulling the naming logic into a helper.
Closes #1336 and closes #1338
Handling it in
linters_with_defaults()alone is pretty painful because of thesubstitute()-based logic for auto-naming, so I implemented the patch insidemodify_defaults()instead.Originally wanted to just throw a warning with
modify_defaults()too, but it seems unlikely to usedefault=by accident there becausedefaults=is a required argument. So added some uglier logic to allowmodify_defaults(default = , ...)to work, assuming that was intentional.