Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle non finite edge cases with pluralization() #716

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

rundel
Copy link
Contributor

@rundel rundel commented Aug 15, 2024

This is a draft resolution to #701 and some related edge cases. The issue seems to be more general than originally described with handling any of the non-finite values (including NAs).

The currently implemented behavior is the following:

  • Numeric non-finite values (NA_real_, NA_integer_, NaN, Inf and -Inf) will be treated as a qty > 1 for pluralization purposes.

  • Non-numeric NA values (NA_character_ and NA) are treated as a non-numeric input so qty is based on the vector's length.

I've opted not to throw an error for numeric NAs or NaN cases since that seemed a bit severe but it also seems like a plausible outcome and happy to switch to that behavior if that is a better option.

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks! The commit suggestions are just ideas and by no means authoritative.

tests/testthat/test-pluralization.R Outdated Show resolved Hide resolved
tests/testthat/_snaps/pluralization.md Show resolved Hide resolved
tests/testthat/_snaps/pluralization.md Show resolved Hide resolved
@krlmlr krlmlr requested a review from hadley August 15, 2024 18:37
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
@gaborcsardi
Copy link
Member

gaborcsardi commented Aug 28, 2024

Looks great, it is exactly the solution I had in mind. :D Thank you!

@gaborcsardi gaborcsardi merged commit b14d61b into r-lib:main Aug 28, 2024
13 checks passed
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.

4 participants