Skip to content
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

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

Open
chainsawriot opened this issue Aug 29, 2024 · 13 comments

Comments

@chainsawriot
Copy link
Collaborator

rio/R/utils.R

Lines 145 to 150 in 447ea1b

lifecycle::deprecate_warn(
when = "1.0.3",
what = "import(trust = 'should be explicit for serialization formats')",
details = paste0("Missing `trust` will be set to FALSE by default for ", format, " in 2.0.0."))
trust <- TRUE ## Change this for version 2.0.0
}

@chainsawriot
Copy link
Collaborator Author

r-lib/lifecycle#187

As well as #450 #432

@chainsawriot
Copy link
Collaborator Author

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

@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Sep 10, 2024

tag @schochastics @jsonbecker

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

@jsonbecker
Copy link
Collaborator

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.

@schochastics
Copy link
Member

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

chainsawriot added a commit that referenced this issue Sep 10, 2024
@chainsawriot chainsawriot reopened this Sep 24, 2024
@chainsawriot
Copy link
Collaborator Author

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

@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Sep 24, 2024

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

@schochastics
Copy link
Member

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

@chainsawriot
Copy link
Collaborator Author

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

@olivroy
Copy link

olivroy commented Sep 25, 2024

Agreed the warning seems fine.

Here is my attempt at explaining caller_env()

wjakethompson/taylor#49 (comment)

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 r-lib/lintr#2602 or gt rstudio/gt#1638

@chainsawriot chainsawriot reopened this Nov 26, 2024
@chainsawriot
Copy link
Collaborator Author

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 chainsawriot changed the title .check_trust(): Is there a way to NOT encouraging users to report the issue here? for lifecycle::deprecate_warn Is there a way to NOT encouraging users to report the issue here? Nov 26, 2024
@olivroy
Copy link

olivroy commented Dec 13, 2024

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.

@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Dec 18, 2024

An experiment with this

rio/R/import_methods.R

Lines 77 to 82 in f1094bf

.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.")
}
import_delim(file = file, sep = if (sep == "|") "auto" else sep, dec = dec, ...)
}

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()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants