Warn on the use of ..var.. and stat(var)

[yutannihilation]

Fix #3693

Considering the wide use of them, now I feel it's a bit difficult to deprecate them. Still, I think it's a good idea to issue a non-warning message at least because we want to advertise after_stat() more.

[thomasp85]

I think this is sensible - @hadley do you have any issues with this?

Also, @yutannihilation is there a reason why you are not using the lifecycle::deprecate_soft() functionality for this?

[yutannihilation]

why you are not using the lifecycle::deprecate_soft() functionality

The main reason is that I just copied the code from here. Actually, I'm not sure when to use lifecycle::* functions and when to use cli::* functions... But, in this case, I think "lifecycle::deprecate_soft()" is not suitable because ..var.. and stat(var) is not deprecated (NEWS.md says they are "superseded").

ggplot2/R/layer.r

Lines 132 to 136 in 7ac55dd

	# TODO: move to cli_warn()
	cli::cli_inform(c(
	"{.field size} aesthetic has been deprecated for use with lines as of ggplot2 3.4.0",
	"i" = "Please use {.field linewidth} aesthetic instead"
	), .frequency = "regularly", .frequency_id = "ggplot-size-linewidth")

[thomasp85]

Ah, I see... The reason why we don't use it in the example above is that the auto-generation of text doesn't really capture what is happening due to the special syntax of ggplot2

In general we will always use the lifecycle functions for deprecations. If they have been superseded I don't think we should issue any warning at all since that means they are still proper syntax

I guess the question then is: Do we want to formally deprecate them? If so we should use lifecycle

[yutannihilation]

I see, thanks for the explanation!

If they have been superseded I don't think we should issue any warning at all since that means they are still proper syntax

I guess the question then is: Do we want to formally deprecate them? If so we should use lifecycle

Makes sense. In my opinion, we should deprecate them then. A superseded function or syntax is preserved because it might not be very easy for all users to replace it with the superseder one. But, in this case, all after_stat(var), ..var.., and stat(var) refer to the same thing with different names. I'll wait for Hadley's comment.

[hadley]

I think it's probably ok to deprecate them; the warning is only once every 8 hours so it's not terribly annoying.

Note that in dev lifecycle you can use I() in what and reason to generate your own text for this sort of situation. lifecycle will be going to CRAN soon, so I think it's ok to temporarily depend on dev lifecycle to get access to this functionality.

[yutannihilation]

Thanks! Then, let's move forward.

But, I found lifecycle::deprecate_soft() doesn't fit in this case because both ..var.. and stat(var) are not functions that are called directly, but are just expressions evaluated non-standardly. My guess is that this was also the true reason why you chose cli_inform() in linewidth's case. Am I correct?

Let me raise one more question. If the intention is to mimic the behaviour of lifecycle::deprecate_soft(), shouldn't we use cli::cli_warn() instead of cli::cli_inform()? (I mean both in this pull request and in the linewidth's case)

[hadley]

See https://lifecycle.r-lib.org/dev/articles/communicate.html#anything-else

I’d also recommend using deprecate_warn() as soft deprecation will only affect package developers (which I don’t think is what you want).

[thomasp85]

@yutannihilation you can easily use lifecycle for deprecate_warn("3.4.0", "stat()", "after_stat()")... it's a bit more involved to use it for ..var.. so there we should rely on dev lifecycle

[yutannihilation]

@hadley
Thanks, you are right. This deprecation targets the end users. It makes sense to skip _soft() stage.

@thomasp85
I see, thanks. I'll use I() for the dot-dot notation.

[hadley]

If you don't mind, could you also update the line size deprecation to use the new lifecycle feature?

[yutannihilation]

Sure!

[yutannihilation]

devtools::load_all("~/GitHub/ggplot2/")
#> ℹ Loading ggplot2

p <- ggplot(data.frame(), aes(..x.., stat(y)))
b <- ggplot_build(p)
#> Warning: The dot-dot notation (`..var..`) was deprecated in ggplot2 3.4.0.
#> Please use `after_stat()` instead.
#> Warning: `stat()` was deprecated in ggplot2 3.4.0.
#> Please use `after_stat()` instead.

^{Created on 2022-08-24 with reprex v2.0.2}

[hadley]

I think you can remove this comment; we're still some time away from setting always = TRUE, if we ever do so. This is a very old feature, so I think the deprecation needs to be quite mild.

[yutannihilation]

Sure.

[yutannihilation]

I needed a tweak here because of this issue: #4960

[hadley]

Could you please add a snapshot test that shows both new warnings?

[hadley]

Could we interpolate the variable name in to ..{{var}}.. and after_stat({var}})? (assuming var <- as.character(x)))

[yutannihilation]

Do you mean this is supposed to work...?

var <- "foo"
lifecycle::deprecate_warn("3.4.0", "a()", "b({{var}})")
#> Error in `as_string()`:
#> ! Can't convert a call to a string.

#> Backtrace:
#>     ▆
#>  1. └─lifecycle::deprecate_warn("3.4.0", "a()", "b({{var}})")
#>  2.   ├─id %||% msg
#>  3.   └─lifecycle:::lifecycle_message(...)
#>  4.     └─lifecycle:::spec(with, NULL, signaller)
#>  5.       └─lifecycle:::spec_arg(what$call, signaller = signaller)
#>  6.         └─rlang::as_string(node_car(arg))
#>  7.           └─rlang:::abort_coercion(x, "a string")
#>  8.             └─rlang::abort(msg, call = call)

^{Created on 2022-08-25 with reprex v2.0.2}

[hadley]

Oh sorry, you’ll need to glue/paste yourself.

[yutannihilation]

I see. Thanks for the suggestion anyway!

[hadley]

Looks great!

[yutannihilation]

Thanks!

@thomasp85
Do you have further comments?

[thomasp85]

LGTM

[yutannihilation]

Thanks!