fix: fct_na_value_to_level() no longer adds unnecessary level when no NAs present (#347)

R/na.R

@@ -39,6 +39,10 @@ fct_na_value_to_level <- function(f, level = NA) {
   f <- check_factor(f)
   check_string(level, allow_na = TRUE)
 
+  if (!any(is.na(f))) {
+    return(f)
+  }
+
   f <- fct_expand(f, NA)
   new_levels <- levels(f)
   new_levels[is.na(new_levels)] <- level

R/reorder.R

@@ -62,6 +62,16 @@ fct_reorder <- function(
 ) {
   f <- check_factor(.f)
   stopifnot(length(f) == length(.x))
+
+  if (missing(.fun) && !is.numeric(.x)) {
+    cli::cli_abort(
+      c(
+        "{.arg .x} must be a numeric vector when using the default {.arg .fun}.",
+        i = "Either supply a numeric {.arg .x} or provide a custom {.arg .fun}."
+      )
+    )
+  }
+
   .fun <- as_function(.fun)
   check_dots_used()
   check_bool(.na_rm, allow_null = TRUE)

tests/testthat/_snaps/reorder.md

@@ -39,6 +39,24 @@
       Error in `fct_reorder()`:
       ! `.desc` must be `TRUE` or `FALSE`, not the number 1.
 
+# fct_reorder() errors with character .x and default .fun (#387)
+
+    Code
+      fct_reorder(f, x)
+    Condition
+      Error in `fct_reorder()`:
+      ! `.x` must be a numeric vector when using the default `.fun`.
+      i Either supply a numeric `.x` or provide a custom `.fun`.
+
+# fct_reorder() errors with factor .x and default .fun (#387)
+
+    Code
+      fct_reorder(f, x)
+    Condition
+      Error in `fct_reorder()`:
+      ! `.x` must be a numeric vector when using the default `.fun`.
+      i Either supply a numeric `.x` or provide a custom `.fun`.
+
 # fct_reorder2() automatically removes missing values with a warning
 
     Code

tests/testthat/test-na.R

@@ -30,6 +30,16 @@ test_that("can turn custom levels into an NA value", {
   )
 })
 
+test_that("does not add level when no NAs present (#347)", {
+  f <- fct(c("a", "b", "c"))
+
+  # With custom level
+  expect_identical(fct_na_value_to_level(f, "x"), f)
+
+  # With default level
+  expect_identical(fct_na_value_to_level(f), f)
+})
+
 test_that("checks input types", {
   f <- fct("a")
   expect_snapshot(error = TRUE, {

tests/testthat/test-reorder.R

@@ -55,6 +55,33 @@ test_that("fct_reorder() validates its inputs", {
   })
 })
 
+test_that("fct_reorder() errors with character .x and default .fun (#387)", {
+  f <- c("a", "b", "b")
+  x <- c("z", "x", "y")
+
+  expect_snapshot(error = TRUE, {
+    fct_reorder(f, x)
+  })
+})
+
+test_that("fct_reorder() errors with factor .x and default .fun (#387)", {
+  f <- c("a", "b", "b")
+  x <- factor(c("z", "x", "y"))
+
+  expect_snapshot(error = TRUE, {
+    fct_reorder(f, x)
+  })
+})
+
+test_that("fct_reorder() works with character .x and custom .fun (#387)", {
+  f <- c("a", "b", "b")
+  x <- c("z", "x", "y")
+
+  # Should work with a custom function that handles character vectors
+  result <- fct_reorder(f, x, .fun = function(x) x[1])
+  expect_equal(levels(result), c("b", "a"))
+})
+
 # fct_reorder2 ------------------------------------------------------------
 
 test_that("can reorder by 2d summary", {