Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Added include_count parameter to pluralize. #3151

Merged
merged 1 commit into from

8 participants

@rroblak

The include_count parameter makes pluralize more flexible by giving the ability to pluralize a word based on a count without necessarily having to include the count at the beginning of the string.

I came across this while trying to generate markup of the form <b>3</b> dogs, which isn't possible without the include_count parameter.

@josevalim
Owner

Thanks for the pull request, but pluralize(1, "blargle", nil, true) does not look good. I would have no idea what the order of the argument means without looking at the docs. We have two options here:

1) make it an option as in pluralize(1, "blargle", :include_count => 1)
2) define a new method called count_pluralize(1, "blargle")

But still insure if it is worth adding as you can easily define it in your apps.

@rroblak

I agree that it does not look good. I was guided by a desire to play nice with existing usages.

It seems silly to me to have this pluralize method without including an option to omit the count. Sure, it's easy to implement in one's apps, but it's even easier if this functionality exists in the framework. A Google search for "rails pluralize without count" reveals that this is a common annoyance.

I would prefer to not have multiple *pluralize methods, so option 1 seems to be best way to go. Within option 1 there are 2 options:

  1. Support 2 method signatures: pluralize(count, singular, plural_form = nil) and pluralize(count, singular, options = {}), where plural_form is expected to be a String and options is expected to be a Hash with optional parameters :plural_form (default nil) and :include_count (default true).

  2. Support only pluralize(count, singular, options = {}). This would break existing usages, which would be a bad thing. But not overloading methods where not strictly necessary is nice.

@et
et commented

Is this really worth it?
Obviously we don't want logic in our views, but compare these two:

"<b>#{count}</b> #{pluralize(count, "dog", :include_count => false)}"

vs the basic condtion

"<b>#{count}</b> #{count == 1 ? "dog" : "dog".pluralize}"

The second version is shorter and expresses the functionality clearly.

Maybe a better solution would be to just tweak the pluralize String method to take an optional parameter of count.

<b>#{count}</b> #{"dog".pluralize(count)}

@stve

Ironically I stumbled upon this exact problem yesterday which prompted me to open #3146 (i've since closed it so we can consolidate discussion here)

I think breaking compatibility is out of the question. We are likely to break many apps by changing the interface and I don't think this use case is important enough to do so.

I had originally suggested an approach similar to what @zenprogrammer implemented, but I think I like the optional parameter to the pluralize inflector as suggested by @et instead.

@rroblak

Agreed. I've reverted my previous changes and added the optional parameter to String#pluralize.

@josevalim
Owner

I like this. It is indeed simpler.

@fxn
Owner

I also like the newest proposal. @zenprogrammer could you please revise the AS core extensions guide and update the PR?

@vijaydev
Collaborator

@zenprogrammer Also please remove the old commits from the PR and keep only the latest one.

@stve

In addition to the count, you would also need to pass an (optional) pluralized version of the word so that it's implementation mirrors that of the pluralize text helper:

"count".pluralize(2, "counters")
# => "counters"
@stve

I've submitted a pull request that supports the optional plural argument in addition to the changes that @zenprogrammer implemented: #3155

@jonleighton
Collaborator

I am happy with the newer proposal. Not the second arg though, that's what custom inflections are for.

@rroblak

@fxn I've revised the AS core extensions guide locally but it doesn't seem appropriate to push it at least until this PR is merged into master. Unless you know more about that process than me?

@vijaydev The old commits have been removed from the PR.

@stve

fair point @jonleighton. I hadn't thought of custom inflections as we were originally approaching this from the standpoint of a view helper.

nice work @zenprogrammer

@fxn
Owner

@zenprogrammer Let me explain: A patch needs to be complete to be merged: implementation, tests, and docs coverage. Guides are official documentation, and this particular PR needs to update the AS guide to reflect the change. Everything in one single patch.

@rroblak

@fxn Thanks for the info. I was mistaken about the process-- I thought I had to submit it to https://github.com/lifo/docrails first.

The AS guide changes have been incorporated into the PR.

@spagalloco Thanks man. Kudos go to you, it was your idea :)

@fxn
Owner

@zenprogrammer no prob, it is a common misunderstanding. Should probably write a blog post clarifying this topic. Thanks for the update!

@josevalim josevalim merged commit 59d6df5 into rails:master
@TildeWill

This commit seems to be missing from Rails 3.1.3... any reason? The functionality and the solution seemed golden.

@josevalim
Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
17 activesupport/lib/active_support/core_ext/string/inflections.rb
@@ -9,14 +9,25 @@
class String
# Returns the plural form of the word in the string.
#
+ # If the optional parameter +count+ is specified,
+ # the singular form will be returned if <tt>count == 1</tt>.
+ # For any other value of +count+ the plural will be returned.
+ #
+ # ==== Examples
# "post".pluralize # => "posts"
# "octopus".pluralize # => "octopi"
# "sheep".pluralize # => "sheep"
# "words".pluralize # => "words"
# "the blue mailman".pluralize # => "the blue mailmen"
# "CamelOctopus".pluralize # => "CamelOctopi"
- def pluralize
- ActiveSupport::Inflector.pluralize(self)
+ # "apple".pluralize(1) # => "apple"
+ # "apple".pluralize(2) # => "apples"
+ def pluralize(count = nil)
+ if count == 1
+ self
+ else
+ ActiveSupport::Inflector.pluralize(self)
+ end
end
# The reverse of +pluralize+, returns the singular form of a word in a string.
@@ -42,7 +53,7 @@ def singularize
def constantize
ActiveSupport::Inflector.constantize(self)
end
-
+
# +safe_constantize+ tries to find a declared constant with the name specified
# in the string. It returns nil when the name is not in CamelCase
# or is not initialized. See ActiveSupport::Inflector.safe_constantize
View
12 activesupport/test/core_ext/string_ext_test.rb
@@ -20,7 +20,7 @@ class Case
class StringInflectionsTest < Test::Unit::TestCase
include InflectorTestCases
include ConstantizeTestCases
-
+
def test_erb_escape
string = [192, 60].pack('CC')
expected = 192.chr + "&lt;"
@@ -64,6 +64,10 @@ def test_pluralize
end
assert_equal("plurals", "plurals".pluralize)
+
+ assert_equal("blargles", "blargle".pluralize(0))
+ assert_equal("blargle", "blargle".pluralize(1))
+ assert_equal("blargles", "blargle".pluralize(2))
end
def test_singularize
@@ -301,13 +305,13 @@ def test_truncate_multibyte
"\354\225\204\353\246\254\353\236\221 \354\225\204\353\246\254 \354\225\204\353\235\274\353\246\254\354\230\244".force_encoding('UTF-8').truncate(10)
end
end
-
+
def test_constantize
run_constantize_tests_on do |string|
string.constantize
end
end
-
+
def test_safe_constantize
run_safe_constantize_tests_on do |string|
string.safe_constantize
@@ -381,7 +385,7 @@ def to_s
test "A fixnum is safe by default" do
assert 5.html_safe?
end
-
+
test "a float is safe by default" do
assert 5.7.html_safe?
end
View
8 railties/guides/source/active_support_core_extensions.textile
@@ -1426,6 +1426,14 @@ The method +pluralize+ returns the plural of its receiver:
As the previous example shows, Active Support knows some irregular plurals and uncountable nouns. Built-in rules can be extended in +config/initializers/inflections.rb+. That file is generated by the +rails+ command and has instructions in comments.
++pluralize+ can also take an optional +count+ parameter. If <tt>count == 1</tt> the singular form will be returned. For any other value of +count+ the plural form will be returned:
+
+<ruby>
+"dude".pluralize(0) # => "dudes"
+"dude".pluralize(1) # => "dude"
+"dude".pluralize(2) # => "dudes"
+</ruby>
+
Active Record uses this method to compute the default table name that corresponds to a model:
<ruby>
Something went wrong with that request. Please try again.