Skip to content

better locale robustness#6144

Merged
MichaelChirico merged 3 commits intomasterfrom
coerceas-locale-robust-test
May 21, 2024
Merged

better locale robustness#6144
MichaelChirico merged 3 commits intomasterfrom
coerceas-locale-robust-test

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico commented May 20, 2024

Closes #6140; follow-up to #6074

Looking more carefully, coerceAs() will receive a factor as-is from R unless it's doing some lazy evaluation. It's only job is to map what its input into the factor structure it receives.

So I don't think coerceAs() is doing anything wrong, rather, the test is poorly structured:

# return a vector where the first element is the first level of the factor, the second is the second level
coerceAs(1:2, factor(c("x","y")))
# coerce c("x", "y") to a factor, picking up levels automatically
factor(c("x","y"))

i.e. in the second case, 'x' will always come first regardless of which level it is, while in the first, the first level always comes first.

Therefore I think the proposed edit (to always supply levels explicitly) is appropriate -- it abstracts the base-owned logic of generating levels away.

An uglier alternative is to use coerceAs(order(c('x', 'y')), factor(c('x', 'y'))). I don't see any real advantage of this approach.

@MichaelChirico MichaelChirico requested a review from jangorecki May 20, 2024 16:23
Copy link
Copy Markdown
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelChirico MichaelChirico merged commit 0da9496 into master May 21, 2024
@MichaelChirico MichaelChirico deleted the coerceas-locale-robust-test branch May 21, 2024 05:17
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.

coerceAs may be doing factor coercion incorrectly

2 participants