Skip to content

fix: error early when .x is non-numeric and default .fun is used (#387)#397

Closed
LeonidasZhak wants to merge 1 commit into
tidyverse:mainfrom
LeonidasZhak:fix/fct-reorder-character-x-387
Closed

fix: error early when .x is non-numeric and default .fun is used (#387)#397
LeonidasZhak wants to merge 1 commit into
tidyverse:mainfrom
LeonidasZhak:fix/fct-reorder-character-x-387

Conversation

@LeonidasZhak

Copy link
Copy Markdown

Problem

fct_reorder() produces incorrect results or confusing errors when .x is a character vector or factor and the default .fun = median is used:

f = c("a", "b", "b")
x = c("z", "x", "y")

# Character .x: silently returns NA with warning, wrong ordering
forcats::fct_reorder(f, x)
#> Warning in mean.default(sort(x, partial = half + 0L:1L)[half + 0L:1L]):
#> argument is not numeric or logical: returning NA
#> [1] a b b
#> Levels: a b  # Wrong! Should be b a

# Factor .x: confusing error about numeric data
x = factor(c("z", "x", "y"))
forcats::fct_reorder(f, x)
#> Error in median.default(x, ...): need numeric data

Solution

Added input validation that errors early with a clear message when .x is not numeric and the default .fun = median is used:

fct_reorder(f, x)
#> 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`.

Users can still use fct_reorder() with character/factor .x by providing a custom .fun:

fct_reorder(f, x, .fun = function(x) x[1])
#> [1] a b b
#> Levels: b a

Changes

  • R/reorder.R: Added validation for .x type when using default .fun
  • tests/testthat/test-reorder.R: Added 3 tests for character, factor, and custom .fun cases
  • tests/testthat/_snaps/reorder.md: Updated snapshots with new error messages

Validation

…yverse#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
@LeonidasZhak

Copy link
Copy Markdown
Author

Withdrawing this because CI is currently failing and I should not ask maintainers to review an unready automated PR. I will only come back with a clean, focused change if useful. Sorry for the noise, and thank you for maintaining the project.

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.

1 participant