Skip to content

fix: pluralize add tests #712

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

Merged
merged 1 commit into from
Aug 30, 2024
Merged

fix: pluralize add tests #712

merged 1 commit into from
Aug 30, 2024

Conversation

alexfreska
Copy link
Member

  • Fixed a bug in the pluralize helper, added tests.

Copy link

vercel bot commented Aug 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
renterd ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2024 5:59pm
4 Skipped Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Aug 30, 2024 5:59pm
explorer-zen ⬜️ Ignored (Inspect) Visit Preview Aug 30, 2024 5:59pm
hostd ⬜️ Ignored (Inspect) Visit Preview Aug 30, 2024 5:59pm
website ⬜️ Ignored (Inspect) Visit Preview Aug 30, 2024 5:59pm

Copy link

changeset-bot bot commented Aug 30, 2024

🦋 Changeset detected

Latest commit: c96f91f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@siafoundation/units Patch
@siafoundation/hostd-react Patch
@siafoundation/renterd-react Patch
@siafoundation/walletd-mock Patch
@siafoundation/walletd-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@telestrial
Copy link
Contributor

@alexfreska Is this change mainly getting at this "children" example? What was the original undesirable behavior?

@alexfreska
Copy link
Member Author

@alexfreska Is this change mainly getting at this "children" example? What was the original undesirable behavior?

@telestrial not specifically, I just added exhaustive tests for all usage patterns. It actually returned a malformed string for 3 of the test cases, so just generally not correct. I "checked" it in the app, should have had these tests from the start. Results with the old method:
Screenshot 2024-08-30 at 1 50 30 PM

@telestrial
Copy link
Contributor

It actually returned a malformed string for 3 of the test cases, so just generally not correct.

Gotcha. Yeah. I think this is a matter of scope: how comprehensive should this be? If we want an all-purpose pluralization function to use freely anywhere and everywhere, it may make sense to use an npm library. There are so so many edge cases and novel rules about pluralization in the English language. Words that add "s" or "es", ones that do neither, ones that just use a different ending altogether, and more. It would probably take a pretty large engineering effort to cover them all. If instead we're looking to cover this vertical of pluralization for specific words in our apps--hosts, renters, etc, it may make sense to do something a bit more explicit: a map of our known needed words and a function that handles that data structure.

For the comprehensive route: I've used this to some success in other projects.

Even that library won't cover everything, though. This is a deceivingly complex problem in the English language.

@alexfreska
Copy link
Member Author

It actually returned a malformed string for 3 of the test cases, so just generally not correct.

Gotcha. Yeah. I think this is a matter of scope: how comprehensive should this be? If we want an all-purpose pluralization function to use freely anywhere and everywhere, it may make sense to use an npm library. There are so so many edge cases and novel rules about pluralization in the English language. Words that add "s" or "es", ones that do neither, ones that just use a different ending altogether, and more. It would probably take a pretty large engineering effort to cover them all. If instead we're looking to cover this vertical of pluralization for specific words in our apps--hosts, renters, etc, it may make sense to do something a bit more explicit: a map of our known needed words and a function that handles that data structure.

For the comprehensive route: I've used this to some success in other projects.

Even that library won't cover everything, though. This is a deceivingly complex problem in the English language.

I wanted to avoid pulling in a dependency, especially one with a dictionary - this API circumvents the complexity by simply having the caller provide the singular and plural words. The benefit being it abstracts all the nested ternary stuff.

@telestrial
Copy link
Contributor

I wanted to avoid pulling in a dependency, especially one with a dictionary - this API circumvents the complexity by simply having the caller provide the singular and plural words. The benefit being it abstracts all the nested ternary stuff.

Totally agree on the library. Just outlining options, even the less great ones. I think your solution should work well but we may find edge cases over time that challenge the parameters here. Maybe not, though! There's so many weird rules to remember/know them all.

@alexfreska
Copy link
Member Author

I wanted to avoid pulling in a dependency, especially one with a dictionary - this API circumvents the complexity by simply having the caller provide the singular and plural words. The benefit being it abstracts all the nested ternary stuff.

Totally agree on the library. Just outlining options, even the less great ones. I think your solution should work well but we may find edge cases over time that challenge the parameters here. Maybe not, though! There's so many weird rules to remember/know them all.

@telestrial 👍🏼 yep we may need to adjust the options as we go. Oddly I couldn't find thaaat many places we needed pluralization - maybe just my grepping didn't catch em.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants