Skip to content

More flexible syntax for warning messages #120

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

Closed
lionel- opened this issue Mar 2, 2022 · 9 comments · Fixed by #129
Closed

More flexible syntax for warning messages #120

lionel- opened this issue Mar 2, 2022 · 9 comments · Fixed by #129

Comments

@lionel-
Copy link
Member

lionel- commented Mar 2, 2022

The challenge is to have a flexible UI that still makes it easy to supply the metadata stored in the warning conditions, that is package, function name, and argument name.

One way to go about this is to allow a length-2 vector whose first element defines the metadata, and the second specifies a warning message.

deprecate_warn(
  "1.0.0",
  c("pkg::foo(bar)", message = "Supplying `bar = \"none\"` is deprecated.")
)
#> Warning message:
#> Supplying `bar = "none"` is deprecated.

We could use ... as a placeholder that lifecycle could fill in with generated contents.

deprecate_warn(
  "1.0.0",
  c("pkg::foo(bar)", message = "Supplying `bar = \"none\"`...")
)
#> Warning message:
#> Supplying `bar = "none"` is deprecated as of pkg 1.0.0.

deprecate_warn(
  "1.0.0",
  c("pkg::foo(bar)", message = "... can no longer be \"none\"...")
)
#> Warning message:
#> The `bar` argument of `foo()` can no longer be "none" as of pkg 1.0.0.

The with argument would also allow a message field:

deprecate_warn(
  "1.0.0",
  what = c("pkg::foo(bar)", message = "Supplying `bar = \"none\"`..."),
  with = c(message = "Please use `bar = NULL` instead.")
)
#> Warning message:
#> Supplying `bar = "none"` is deprecated as of pkg 1.0.0.
#> Please use `bar = NULL` instead.

I like that this leverages existing syntax for defining a deprecation message + metadata.

What do you think @hadley?

@hadley
Copy link
Member

hadley commented Mar 2, 2022

Do you have a few more examples where the current syntax isn't sufficient? Or is this specifically about deprecating values?

@lionel-
Copy link
Member Author

lionel- commented Mar 3, 2022

The current syntax is sufficient when an argument or a function as a whole is being deprecated. Problems arise when values are no longer supported, or when some behaviour is no longer supported. We've added support for fn(arg = "no longer supports foo"), but that is rather inflexible in terms of message generation.

Also the current syntax can't be used when it's not about a function but e.g. a global option. We could extend what in the future to support things like "options(foo = value)", but the proposal above would already offer an alternative by supplying:

what = c(message = "Setting the `foo` global option to value...")

Here is an example from purrr where a more flexible UI would help:

deprecate_warn(
  "0.3.0",
  what = c("list_modify(...)", message = "Removing elements with `NULL`..."),
  with = c(message = "Please use `zap()` to remove elements.")
)
#> Warning message:
#> Removing elements with `NULL` is deprecated as of purrr 0.3.0.
#> Please use `zap()` to remove elements.

oh another thing we should do is include call in the warning. Then we get:

#> Warning message:
#> In list_modify() : Removing elements with `NULL` is deprecated as of purrr 0.3.0.
#> Please use `zap()` to remove elements.

(With the extra space and lack of backticks because we are not in control of warning display.)

@hadley
Copy link
Member

hadley commented Mar 9, 2022

Why can't we use I() like you proposed in slack?

deprecate_warn(
  "0.3.0",
  what = I("Removing elements with `NULL`"),
  with = I("Please use `zap()` to remove elements.")
)

@lionel-
Copy link
Member Author

lionel- commented Mar 14, 2022

Because we need a way to supply metadata in a structured way, i.e. the function name and argument name. This may be useful in the future to inspect properties of warnings. Also, I'm thinking about including the function name as call field, which would make this data immediately useful.

Are you concerned only about the UI or also to the idea of using ... to let lifecycle generate parts of the message?

@hadley
Copy link
Member

hadley commented Mar 14, 2022

Both the UI and and the use of ... seem a bit fiddly to me. Given that we only need to make these exceptions pretty rarely, maybe it's ok if they lack the structured data that most lifecycle condition would have?

@DavisVaughan
Copy link
Member

Also the current syntax can't be used when it's not about a function but e.g. a global option

Just had a situation in yardstick where I wanted to deprecate the yardstick.event_first global option with lifecycle, but couldn't because that wasn't supported

@hadley
Copy link
Member

hadley commented Aug 11, 2022

Another case is r-lib/tidyselect#169 — we'd like to be able to say "Using .data[[var]] in tidyselect is deprecated, use any_of(var) instead."

@hadley
Copy link
Member

hadley commented Aug 13, 2022

One more argument in favour of the I() proposal — we need this sort of deprecation for cases when we're not exactly deprecating a function or an argument, so capturing this information in structured metadata is of less use.

Here are the three examples mentioned above in the thread:

  • yardstick needs to deprecate a global option
  • tidyselect needs to deprecate use of .data in a specific context
  • purrr needs to deprecate use of NULL inside of ... in list_modify()

Maybe there's a distinction between static and dynamic deprecation? In the vast majority of cases, you could identify the deprecation with a static analysis because we either deprecate an entire function or a specific argument. That's what lifecycle is currently designed for and it does a great job at it. In other cases, like in this issue, we have a deprecation that can only be detected dynamically, and lifecycle isn't such a good fit.

Dynamic deprecations tend to be much rarer (~5 vs. ~80) so ideally we could use the existing interface with some minor changes.

@lionel-
Copy link
Member Author

lionel- commented Aug 15, 2022

Sounds good, let's go with I() and we can think of something more structured in the future if we find a need for it (e.g. analytics).

hadley added a commit that referenced this issue Aug 15, 2022
hadley added a commit that referenced this issue Aug 16, 2022
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

Successfully merging a pull request may close this issue.

3 participants