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

Quote(s) -> pluralized properly #531

Merged
merged 5 commits into from
Nov 19, 2023
Merged

Quote(s) -> pluralized properly #531

merged 5 commits into from
Nov 19, 2023

Conversation

MrOnosa
Copy link
Contributor

@MrOnosa MrOnosa commented Nov 18, 2023

It's a pet-peeve of mine.

It's a pet-peeve of mine.
@MrOnosa
Copy link
Contributor Author

MrOnosa commented Nov 18, 2023

That's what I get for trying to program on my cell phone.

@MrOnosa MrOnosa marked this pull request as draft November 19, 2023 00:01
@Ixrec
Copy link
Contributor

Ixrec commented Nov 19, 2023

For what it's worth, if we want to be fancy about plural handling, what we'd really want is a little helper method like pluralize(number, "thing", "things"). It may be a trivial method, but over all the "(s)"s in a codebase like this (or in the one at my day job which does have such a helper) having a standard name for it and slightly less logic at the string interpolation site adds up to a significant benefit.

@MrOnosa
Copy link
Contributor Author

MrOnosa commented Nov 19, 2023

And that's what I get for not just forking the project because it was working locally but I forgot to update all the files using the GitHub editor.

Noted on using pluralize. Were you thinking of a full library, like Pluralize.NET, or just a simple helper method?

@MrOnosa MrOnosa marked this pull request as ready for review November 19, 2023 00:51
@Ixrec
Copy link
Contributor

Ixrec commented Nov 19, 2023

Oh I just meant a simple helper method for us. I don't think Izzy's usage of plural words is nearly advanced or dynamic enough to benefit from what Pluralize.NET is doing.

Copy link
Contributor

@Ixrec Ixrec left a comment

Choose a reason for hiding this comment

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

just a few more small fixes

Izzy-MoonbotTests/Tests/QuoteModuleTests.cs Outdated Show resolved Hide resolved
Izzy-MoonbotTests/Tests/QuoteModuleTests.cs Outdated Show resolved Hide resolved
@MrOnosa MrOnosa requested a review from Ixrec November 19, 2023 14:19
@Ixrec Ixrec merged commit 009cbe6 into mane Nov 19, 2023
3 checks passed
@Ixrec Ixrec deleted the MrOnosa-patch-1 branch November 19, 2023 14:36
@ParaSwarm
Copy link

@MrOnosa you're a peach and a warrior of light

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.

3 participants