rio icon indicating copy to clipboard operation
rio copied to clipboard

for `lifecycle::deprecate_warn` Is there a way to NOT encouraging users to report the issue here?

Open chainsawriot opened this issue 1 year ago • 13 comments

https://github.com/gesistsa/rio/blob/447ea1b9b2f50761f8a646d4adb95e2a270175c0/R/utils.R#L145-L150

chainsawriot avatar Aug 29 '24 12:08 chainsawriot

r-lib/lifecycle#187

As well as #450 #432

chainsawriot avatar Aug 29 '24 12:08 chainsawriot

#452 I think we need to address this before the typical "semester beginning" wave.

chainsawriot avatar Sep 10 '24 14:09 chainsawriot

tag @schochastics @jsonbecker

I think one solution is to not use lifecycle for this. Just use the good old warning().

chainsawriot avatar Sep 10 '24 14:09 chainsawriot

Oh wow that's aggressive to report an issue to a maintainer when they've deprecated a feature?

I'm good with swapping this to warning but that's a bad upstream choice.

jsonbecker avatar Sep 10 '24 14:09 jsonbecker

That is indeed a bit aggressive. I second that a good old warning is enough.

schochastics avatar Sep 10 '24 18:09 schochastics

Maybe should give this suggestion by @olivroy a try: r-lib/lifecycle#187

chainsawriot avatar Sep 24 '24 13:09 chainsawriot

But how deep in the caller_env() should we go? It is still quite convoluted.

chainsawriot avatar Sep 24 '24 14:09 chainsawriot

I think I still vote for an ordinary warning but mostly because I have not yet fully understood the caller_env()

schochastics avatar Sep 24 '24 18:09 schochastics

@schochastics OK, I agree with you. And I close this once again.

chainsawriot avatar Sep 25 '24 08:09 chainsawriot

Agreed the warning seems fine.

Here is my attempt at explaining caller_env()

https://github.com/wjakethompson/taylor/pull/49#issuecomment-2197134362

This guide can be helpful too.

https://rlang.r-lib.org/reference/topic-error-call.html

I made a few PRs to implement this in different packages.

See lintr for example https://github.com/r-lib/lintr/pull/2602 or gt https://github.com/rstudio/gt/pull/1638

olivroy avatar Sep 25 '24 14:09 olivroy

I am reopening this because of this issue #459

We need to stop emitting the issue reporting link. The last solution was not a real solution.

chainsawriot avatar Nov 26 '24 14:11 chainsawriot

Well, in the case of #459. It is technically correct to report to {rio}. But {rio} should probably emit one of its own warnings also. Once {haven} decides to deprecate its function, it will break the {rio} package, not just user code...

The sooner {rio} deprecates it on its side, the safer it is for both {rio} users and maintainers.

olivroy avatar Dec 13 '24 13:12 olivroy

An experiment with this

https://github.com/gesistsa/rio/blob/f1094bf636f95f5dfcb05408d2ca0960fa792c8e/R/import_methods.R#L77-L82

If I change it to

.import.rio_psv <- function(file, sep = "|", which = 1, fread = lifecycle::deprecated(), dec = if (sep %in% c("|", "auto")) "." else ",", ...) {
    if (lifecycle::is_present(fread)) {
        lifecycle::deprecate_warn(when = "0.5.31", what = "import(fread)", details = "psv will always be read by `data.table:fread()`. The parameter `fread` will be dropped in v2.0.0.", user_env = rlang::caller_env(3))
    }
    import_delim(file = file, sep = if (sep == "|") "auto" else sep, dec = dec, ...)
}

it doesn't emit the url. rlang::call_env(3) is the same as parent.frame(4).

But it can't be tested with

lifecycle::expect_deprecated()

chainsawriot avatar Dec 18 '24 12:12 chainsawriot