-
Notifications
You must be signed in to change notification settings - Fork 26
Bring back fast signal_stage()
#203
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately git diff thinks this is a "new" file
signal_experimental <- function(when, what, env = caller_env()) { | ||
deprecate_soft( | ||
"1.1.0", | ||
what = "signal_experimental()", | ||
with = "signal_stage()", | ||
id = "lifecycle_signal_experimental" | ||
) | ||
signal_stage("experimental", what, with = NULL, env = env) | ||
} | ||
|
||
#' @rdname deprecated-signallers | ||
#' @export | ||
signal_superseded <- function(when, what, env = caller_env()) { | ||
deprecate_soft( | ||
"1.1.0", | ||
what = "signal_superseded()", | ||
with = "signal_stage()", | ||
id = "lifecycle_signal_superseded" | ||
) | ||
signal_stage("superseded", what, with = NULL, env = env) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did leave these as officially soft-deprecated
Latest commit tries another approach - making This makes it clear that it is purely about expressing intent at the call site. It is also obviously as fast as it gets. library(lifecycle)
bench::mark(
signal_stage("experimental", "this()"),
iterations = 100000
)
#> # A tibble: 1 × 6
#> expression min median `itr/sec` mem_alloc `gc/sec`
#> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl>
#> 1 "signal_stage(\"experimental\", \"this()\")" 82ns 205ns 3736394. 0B 0 I did mark the arg |
In #202 we decided to deprecate
signal_stage()
, but as we thought more about it, it seems like this:is quite useful for expressing intent at the call site, even if it doesn't do anything.
We've certainly used this in dplyr, purrr, recipes, and ggplot2, and the community has as well:
https://github.com/search?q=%22lifecycle%3A%3Asignal_stage%22+org%3Acran&type=code
So we've decided to keep it after all, with a few tweaks:
The
conditionMessage()
method emits""
, for maximum performance, since it can't be lazy right now. See inline comment below.We do not use
signal_stage()
indeprecate_*()
anymore, for max performance. We also think that if we were to do anything with these functions in the future, it would probably be some kind of static analysis, and we could look for bothsignal_stage()
anddeprecate_*()
call sitesRelated to the above,
"deprecated"
is no longer a validstage
forsignal_stage()
. This was never documented to be valid, it was only used internally. No one seems to have used this.There is no currently exported way to get at the "data" in the condition itself. It is now purely used to express intent at the call site. We can consider adding something in the future if needed, but it seems unlikely.
Remembering that
signal_stage("experimental", ...)
will be used in our experimental functions and we don't want a performance penalty there, I'm happy with this performance: