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

Allow pluralization/singularization to be controlled by a boolean. #25

Closed
wants to merge 1 commit into from
Closed

Conversation

Royce
Copy link

@Royce Royce commented Nov 6, 2013

I would like your thoughts on being able to control these extension methods with a bool like:

string.Format("{0} {1}", count, "request".Pluralize(count != 1));

Maybe I should take it further to be:

"request".Pluralize(count); // "0 requests", "1 request", "2 requests"

@MehdiK
Copy link
Member

MehdiK commented Nov 8, 2013

Thanks for the contribution. I like it. I will check it out soon and will get back to you.

@MehdiK
Copy link
Member

MehdiK commented Nov 9, 2013

On a second thought I just had a look and there are a bunch of other methods that look and behave like these two methods. I think having the condition on two methods and not on others makes the API inconsistent. So I guess I have to say no to this!

If you need this on your code, you could obviously create a simple extension method that checks for the bool value and conditionally calls Humanizer's Pluralize etc.

@MehdiK MehdiK closed this Nov 9, 2013
@Royce
Copy link
Author

Royce commented Nov 11, 2013

Would you be interested in me adding bool onlyIf = true to all relevant parts of the API? On reflection I am starting to think this is a little gross.

Regardless, I am still curious what you think of:

"request".Pluralize(count); // "0 requests", "1 request", "2 requests"

I seem to find myself implementing this sort of logic in nearly every web app I work on, or resorting to "x request(s)".

@MehdiK
Copy link
Member

MehdiK commented Nov 11, 2013

Frankly I don't like onlyIf added to a lot of methods - feels like unnecessary noise.

I quite agree about the need for a smarter Pluralize though. Lets leave the existing method alone and create an overload with count.

A separate PR? :)

@Royce
Copy link
Author

Royce commented Nov 11, 2013

Happily. 


Royce Townsend

On Mon, Nov 11, 2013 at 4:52 PM, Mehdi Khalili notifications@github.com
wrote:

Frankly I don't like onlyIf added to a lot of methods - feels like unnecessary noise.
I quite agree about the need for a smarter Pluralize though. Lets leave the existing method alone and create an overload with count.

A separate PR? :)

Reply to this email directly or view it on GitHub:
#25 (comment)

@breyed
Copy link

breyed commented Dec 20, 2013

Using a count rather than a boolean is much more flexible. English is simple compared to other languages in that it only has two conjugations of a noun based on the number. Other languages vary - quite a bit, actually. Fortunately, there is a lot of work done on this already. Take a look at Quantity Strings in the Android documentation. It's very well thought out.

Based on my experience on Android, I'd recommending keeping Pluralize and Singularize as they are, and adding a ToQuantity extension method, which takes a string and an integer.

@MehdiK
Copy link
Member

MehdiK commented Dec 20, 2013

Sounds good @breyed. I actually like ToQuantity better than an overload for Pluralize.

What do you think @Royce ?

@Royce
Copy link
Author

Royce commented Dec 21, 2013

ToQuantity sounds subtly wrong to me as one as adjoining a quantity, rather than converting to a quantity. I wonder if WithQuantity(...) is more descriptive.

ToQuantity is probably more discoverable. A quantity extension sounds better to me than a pair of pluralize/singularize overloads. 


Royce Townsend

On Fri, Dec 20, 2013 at 11:03 PM, Mehdi Khalili notifications@github.com
wrote:

Sounds good @breyed. I actually like ToQuantity better than an overload for Pluralize.

What do you think @Royce ?

Reply to this email directly or view it on GitHub:
#25 (comment)

@hazzik
Copy link
Member

hazzik commented Dec 21, 2013

I would prefer AsQuantity

@breyed
Copy link

breyed commented Dec 21, 2013

The convention in the .NET Framework is to use "As" when the result is another view into an existing object and "To" when the result is a new object. The extension methods on IEnumerable serve as good examples. Since the quantity method returns a new string object, per convention, the prefix should be "To".

@MehdiK
Copy link
Member

MehdiK commented Dec 22, 2013

I think both As and To will work & happy with both. @breyed has a good point on .Net conventions tho so I vote for ToQuantity.

@sinairv
Copy link

sinairv commented Dec 22, 2013

Although @breyed 's point is quite relevant in terms of consistency with .NET Framework's existing API design, I found WithQuantity more fluent (i.e., more appropriate for a fluent API).

@breyed
Copy link

breyed commented Dec 22, 2013

The Humanizer methods are a conversion API. The With prefix is used in configuration APIs, such as those in Entity Framework. A configuration API returns the same object successively or objects that tie back to common data, which successively build up configuration in preparation for a coordinated operation.

By contrast, in conversion APIs, each operation is distinct, and order can be significant. For example, System.String is a conversion API. s.Replace("a","b").ToLower() produces a different result than s.ToLower().Replace("a","b"), and in either case, and you get a new result - not a configuration object - at each step along the chain.

Humanizer is similar. "AirCraft".Humanize().Pluralize() produces a different result than "AirCraft".Pluralize().Humanize(), and you get a new result at each step along the chain.

Since Humanizer is a conversion API, I think it is better to model it after others like it such as System.String and System.DateTime.

@sinairv
Copy link

sinairv commented Dec 22, 2013

Great tips @breyed. Now I see that With suits more the builder pattern, not a conversion API. Thanks.

@MehdiK
Copy link
Member

MehdiK commented Dec 23, 2013

@breyed has good points that are hard to argue with. So let's go with ToQuantity.

Thanks a lot everyone for your inputs & thoughts.

@Royce
Copy link
Author

Royce commented Dec 23, 2013

I still feel that ToQuantity is not perfect. The behaviour of this method (that takes a string/noun and a integer/quantity) is less like ToLower/ToInt/ToString and more like PadLeft(Int32). Furthermore the method name ToQuantity sounds like it returns a quantity rather than have a quantity as an argument. I accept that AsXX and WithXXX are misleading according to .Net conventions.

Just because our method returns a new string doesn't mean it has to start withTo. What about:

  • JoinWithQuantity(int)
  • Join(int)
  • AddQuantity(int)
  • Or something better, please!?

@MehdiK
Copy link
Member

MehdiK commented Dec 24, 2013

I agree that ToQuantity is not perfect and I thought about the perceived meaning of the method like you say; but AsQuantity has the same issue and while I don't really enjoy ToQuantity I personally couldn't come up with a better name. The only other alternative that came to mind, which is also in line with a lot of Humanizer methods, was Quantitize! Although not a real word, I personally like it better. That said I have a long history of creating meaningless words (e.g. BDDfy, Seleno) and I've been told by English speakers to stop effing with the language ;)

@breyed
Copy link

breyed commented Dec 25, 2013

I too agree ToQuantity would be better if its name indicated that it returns a localized string, not an integer. More explicit names like ToQuantityWords or ToQuantityText would do it, with the former having the advantage of consistency with the existing Humanizer methods ToOrdinalWords and ToWords that convert from integers to strings. However, I don't believe the added clarity justifies the wordiness, especially since Intellisense readily displays the return type.

An ize suffix generally has the same meaning as To, e.g. Quantitize == ToQuantity, Singularize == ToSingular, Lowerize == ToLower. Sometimes, this doesn't hold, e.g. the closest equivalent to Humanize would be something more wordy, such as ToHumanFriendlyWords. When the plain English is compact (and often even if not), it's the best choice. While invented words are cute, they become API special cases, which don't scale well. Imagine if every team that contributed to the .NET Framework and every NuGet package had its own cuteness: the novelty would wear off quickly and we'd be left with dozens of unnecessary learning curves for any significant-sized project. Ideally, all the different libraries we use together every day should sign together like a beautiful choir. When adding extension methods to high profile classes such as System.String, it's important to be especially careful that the voices blend well.

@MehdiK
Copy link
Member

MehdiK commented Jan 1, 2014

ToQuantity was implemented in #37 and was patched in #43. Will release soon.

I honestly don't mind redundant methods if we ever find a much better clearer method name, and will add that in.

@MehdiK
Copy link
Member

MehdiK commented Jan 1, 2014

Oh, and thanks a lot everyone for your great inputs. I loved this thread.

@breyed
Copy link

breyed commented Jan 1, 2014

I realize we're talking hypothetical here, but I'd caution against redundant methods. I've thought so ever since I read Effective C++ by Scott Meyers many years ago. It contains a principle Strive for interfaces that are minimal and complete, which applies equally well to .NET. That said, it's no fun being saddled with legacy APIs (e.g. one of the few things I dislike about Python is how it borrowed so much of the baggage from the C standard library). So should a better idea come around, I'm all in favor into making it available. However, in general, I'd try to bundle a number of changes together into a version 2 of the API, and put the new API into a different namespace. This would keep compatibility with old code while allowing new code to use the new API, which would still be minimal and complete.

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.

5 participants