-
Notifications
You must be signed in to change notification settings - Fork 26
Deprecate signal_stage()
#202
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
And remove all usage of it internally
expect_warning(eval_bare(call2(fn), env(global_env())), "was deprecated") | ||
}) | ||
|
||
test_that("deprecation warnings are not displayed again", { |
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.
This test was super broken and wasn't testing what we thought it was. Mostly due to the deprecate_warn()
calls needing to be wrapped in an actual function to simulate real usage and make them warn the right way.
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.
Make sense to me.
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.
If you don't supply a what
but still supply an id
it's still very fast right?
Relatedly I think it's worth adding an annotation or comment of some kind to deprecate_warn0()
that msg
is passed lazily. The default assumption for R code should be that the timing of evaluation doesn't matter, so going against that should be declared/documented.
@@ -1,29 +1,45 @@ | |||
#' Signal other experimental or superseded features | |||
#' Deprecated functions for signalling lifecycle stages |
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'm not sure it's worth deprecating these functions.)
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.
Decided over Zed to go ahead and soft-deprecate them with the goal of having an easy removal in a few years
Co-authored-by: Lionel Henry <lionel.hry@proton.me>
Closes #198 - Dark hackery no longer needed
Closes #199 -
signalCondition()
no longer neededCloses #162 - No longer need this if
signal_stage()
is goneCloses #91 - No longer need this if
signal_stage()
is goneCloses #176 - It's very fast now
Closes #158 - No longer needed if we have no way to report on superseded functions.
This PR soft-deprecates
signal_stage()
and removes usage of it fromdeprecate_soft()
,deprecate_warn()
, anddeprecate_stop()
, so they are now very fast after all my other work here.It also officially soft-deprecates
signal_experimental()
andsignal_superseded()
, so they can eventually be removed alongsidesignal_stage()
.TLDR: In the tidyverse we should really encourage including a fixed
id
in ourdeprecate_soft()
anddeprecate_warn()
calls. It makes them much much faster.If you're using
id
and aren't usingalways = TRUE
, it is VERY fast now. This is what dplyr and friends can start using everywhere for alldeprecate_soft()
anddeprecate_warn()
calls.If you aren't using
id
, then we have to recreate yourmessage
every time to have a unique key, so it's a bit slower, but still quite a bit faster than where we were!Here's an example with AsIs messages that don't require any
what
expression parsingIf you are using the expression parsing features of lifecycle and you aren't using a static
id
, then it's still not super fast. But remember this problem goes away entirely if you use a staticid
!