From 309e7a88d6099b6efcecc22563a9ace97fc4ad98 Mon Sep 17 00:00:00 2001 From: Leonidas Zhak <70497898+LeonidasZhak@users.noreply.github.com> Date: Sat, 6 Jun 2026 22:37:02 +0800 Subject: [PATCH 1/2] fix: error early when .x is non-numeric and default .fun is used (#387) - Add input validation for .x when using default median function - Provide clear error message suggesting numeric .x or custom .fun - Add tests for character and factor .x inputs - Add test showing custom .fun works with character .x --- R/reorder.R | 10 ++++++++++ tests/testthat/_snaps/reorder.md | 18 ++++++++++++++++++ tests/testthat/test-reorder.R | 27 +++++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/R/reorder.R b/R/reorder.R index d3b5fd6b..ec9d7b17 100644 --- a/R/reorder.R +++ b/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) diff --git a/tests/testthat/_snaps/reorder.md b/tests/testthat/_snaps/reorder.md index 23056cf1..32c33a8f 100644 --- a/tests/testthat/_snaps/reorder.md +++ b/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 diff --git a/tests/testthat/test-reorder.R b/tests/testthat/test-reorder.R index 2fdbd69b..b634cb24 100644 --- a/tests/testthat/test-reorder.R +++ b/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", { From 0f01d1406cdd3a43d39caec67830eacb0c7900d4 Mon Sep 17 00:00:00 2001 From: Leonidas Zhak <70497898+LeonidasZhak@users.noreply.github.com> Date: Sat, 6 Jun 2026 23:19:59 +0800 Subject: [PATCH 2/2] fix: fct_na_value_to_level() no longer adds unnecessary level when no NAs present (#347) When called on a factor with no NA values, fct_na_value_to_level() previously added an unnecessary level (either NA or the custom level). Now it returns the factor unchanged, matching the behavior of the deprecated fct_explicit_na(). --- R/na.R | 4 ++++ tests/testthat/test-na.R | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/R/na.R b/R/na.R index 4e5119c4..ca80fd3b 100644 --- a/R/na.R +++ b/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 diff --git a/tests/testthat/test-na.R b/tests/testthat/test-na.R index c09ae0d5..5c867a1b 100644 --- a/tests/testthat/test-na.R +++ b/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, {