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

Supersede recode() and recode_factor() in favor of case_match() #6433

Merged
merged 4 commits into from
Aug 26, 2022

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Aug 25, 2022

@hadley I didn't know if you wanted to soft-deprecate or just signal them as superseded, so I went with superseded like you did with transmute().

I used the example section as a way to show how to transition to case_match() or fct_recode() and to point out the gap that the upcoming forcats function will fill

Comment on lines 60 to +61
#' recode(char_vec, a = "Apple", b = "Banana")
#' case_match(char_vec, "a" ~ "Apple", "b" ~ "Banana", .default = char_vec)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I've written this, I do get the vibe that replace_match() and replace_when() might be useful as even more direct replacements for recode(), but I still can't decide if adding two more functions to dplyr is worth it or not

@DavisVaughan DavisVaughan requested a review from hadley August 25, 2022 22:25
@@ -1,5 +1,10 @@
# dplyr (development version)

* `recode()` is superseded in favor of `case_match()`. `recode_factor()` is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could deprecate since recode() is more obviously wrong, and we've marked as questioning for some time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess I've thought of it as questioning, but we never actually marked it as such 😬

#' use `recode_factor()`, which will change the order of levels to match
#' the order of replacements. See the [forcats](https://forcats.tidyverse.org/)
#' package for more tools for working with factors and their levels.
#' `recode()` is superseded in favor of [case_match()], which handles the most
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should also mention the major problem with recode() is that it has old = new which is the opposite pattern to new = old which we see almost everywhere else.

Copy link
Member Author

@DavisVaughan DavisVaughan Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But case_when() and case_match() are also old ~ new 😬. I think they work that way because that is how it works in SQL. It is also how switch() works

For those reasons, I didn't mention that part

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but it also somehow feels different because of the matching context.

R/recode.R Show resolved Hide resolved
tests/testthat/test-recode.R Show resolved Hide resolved
@DavisVaughan DavisVaughan merged commit f2fb03c into tidyverse:main Aug 26, 2022
@DavisVaughan DavisVaughan deleted the feature/deprecate-recode branch August 26, 2022 13:33
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 this pull request may close these issues.

2 participants