Use rlang to coerce fct_relabel input to function (#91) #112

alistaire47 · 2018-02-12T00:35:57Z

This PR changes fct_relabel per issue #91 so it uses rlang::as_function to treat the fun parameter, enabling the use of string names of functions, formula functions, and quosure functions.

Notes on changes:

As discussed in the issue, this introduces an rlang Imports dependency.
I dropped the sanity check on whether fun is a function, as as_function will always error beforehand anyway.
- I changed the accompanying test to reflect the new behavior, but given that as_function error messages could change, it may be better to drop the test altogether. For now, I left it with just "function" as a regex, which is broad enough that it will probably never break even if errors do change.
I added a test to make sure it works with something that requires coercion to a function.
I changed the parameter documentation to reflect the change. It uses the word "coercible", which devtools::spell_check doesn't like with my current dictionary, but it is a word.
I added an example of the new behavior.
I updated NEWS.md.

All tests and checks pass on MacOS.

Let me know if you'd like any reversions or further changes!

hadley

Thanks! Unfortunately while the code looks good, the docs and tests need a few tweaks.

hadley · 2018-02-12T14:56:20Z

NEWS.md

@@ -1,5 +1,8 @@
 # forcats 0.2.0.9000

+# `fct_relabel()` now accepts objects coercible to functions 


This should start with *, not #

hadley · 2018-02-12T14:56:47Z

NEWS.md

@@ -1,5 +1,8 @@
 # forcats 0.2.0.9000

+# `fct_relabel()` now accepts objects coercible to functions 
+  by `rlang::as_function` (#91)


Can you please add your github user name as described in http://style.tidyverse.org/news.html#during-development ?

hadley · 2018-02-12T14:57:04Z

DESCRIPTION

@@ -16,7 +16,8 @@ Depends:
    R (>= 3.1)
 Imports: 
    magrittr,
-    tibble
+    tibble,


These should be in alphabetical order (that should resolve the merge conflict)

hadley · 2018-02-12T14:57:43Z

R/relabel.R

-#' @param fun A function that is applied to each level. Must accept one
-#'   character argument and return a character vector of the same length as its
-#'   input.
+#' @param fun A function or something coercible to one by [rlang::as_function()]


I think it's more useful to describe exactly what it can be here, rather than requiring the user to read more technical documentation.

hadley · 2018-02-12T14:58:48Z

R/relabel.R

@@ -22,10 +22,14 @@
 #'
 #' rincome2 <- fct_relabel(gss_cat$rincome, convert_income)
 #' fct_count(rincome2)
+#'
+#'


Can you please think through the use of blank lines here? Generally they should be used to group together related code. (This may require tweaking to the entire example block)

I changed the new example to pipes and put the function definition at the start of the second one, so now each example has no blank lines within it, and there's one between them. The fct_count(gss_cat$rincome) doesn't have to come after the function definition, but I think it's more readable in terms of whitespace.

hadley · 2018-02-12T14:59:09Z

R/relabel.R

+#'
+#' fct_count(gss_cat$partyid)
+#'
+#' partyid2 <- fct_relabel(gss_cat$partyid, ~gsub(",", ", ", .x))


I think this example should come first because it's simpler

hadley · 2018-02-12T14:59:39Z

tests/testthat/test-fct_relabel.R

+test_that("function coercion", {
+  f1 <- factor(letters)
+
+  expect_identical(fct_relabel(f1, ~paste0(.x, ".")),


Can you please use the formatting described at http://style.tidyverse.org/syntax.html#long-lines ?

hadley · 2018-02-12T14:59:50Z

tests/testthat/test-fct_relabel.R

@@ -36,3 +36,10 @@ test_that("additional arguments", {

  expect_identical(fct_relabel(f1, paste0, "."), factor(paste0(letters, ".")))
 })
+
+test_that("function coercion", {


This should be a sentence

The others in this file are concise. It's now a sentence, though not capitalized and with a period. If you want that (or for the others to be changed) let me know.

hadley · 2018-02-14T23:27:48Z

Thanks!

For future reference, if you include Fixes #xyz in a commit message (or the text of the PR), the corresponding issue will be automatically closed when the PR is merged.

Use rlang to coerce fct_relabel input to function (tidyverse#91)

382500d

hadley reviewed Feb 12, 2018

View reviewed changes

alistaire47 and others added 2 commits February 12, 2018 14:50

cleaned documnentation and standardized code styling

9f333f4

Merge branch 'master' into master

9126935

hadley merged commit 6e5249f into tidyverse:master Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rlang to coerce fct_relabel input to function (#91) #112

Use rlang to coerce fct_relabel input to function (#91) #112

alistaire47 commented Feb 12, 2018

hadley left a comment

hadley Feb 12, 2018

hadley Feb 12, 2018

hadley Feb 12, 2018

hadley Feb 12, 2018

hadley Feb 12, 2018

alistaire47 Feb 12, 2018 •

edited

Loading

hadley Feb 12, 2018

hadley Feb 12, 2018

hadley Feb 12, 2018

alistaire47 Feb 12, 2018

hadley commented Feb 14, 2018

		@@ -1,5 +1,8 @@
		# forcats 0.2.0.9000

		# `fct_relabel()` now accepts objects coercible to functions

Use rlang to coerce fct_relabel input to function (#91) #112

Use rlang to coerce fct_relabel input to function (#91) #112

Conversation

alistaire47 commented Feb 12, 2018

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alistaire47 Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented Feb 14, 2018

alistaire47 Feb 12, 2018 •

edited

Loading