Make pluralize follow singularize's logic of uncountability check #3536

Merged
merged 1 commit into from Nov 6, 2011

Conversation

Projects
None yet
3 participants
@pksunkara
Contributor

pksunkara commented Nov 6, 2011

I have added a test case which fails prior to the patch. All the tests pass including the new one now.

@josevalim

View changes

activesupport/lib/active_support/inflector/methods.rb
@@ -23,7 +23,7 @@ module ActiveSupport
def pluralize(word)
result = word.to_s.dup
- if word.empty? || inflections.uncountables.include?(result.downcase)
+ if word.empty? || inflections.uncountables.any? { |inflection| result =~ /\b#{inflection}\Z/i }

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 6, 2011

Contributor

Thanks for the pull request. Shouldn't we move this logic to a private method and have both pluralize and singularize calling it?

@josevalim

josevalim Nov 6, 2011

Contributor

Thanks for the pull request. Shouldn't we move this logic to a private method and have both pluralize and singularize calling it?

This comment has been minimized.

Show comment Hide comment
@sarahhodne

sarahhodne Nov 6, 2011

Contributor

That would be DRYer. To me the two methods look almost exactly the same, except for the word.empty? part, so maybe we could extract most of the method into a private method and redefine pluralize to this:

def pluralize(word)
  somethingize(word, inflections.plurals)
  # Alternatively:
  somethingize(word, :plurals) # (and just do inflections.send(:plurals) in somethingize)
end
@sarahhodne

sarahhodne Nov 6, 2011

Contributor

That would be DRYer. To me the two methods look almost exactly the same, except for the word.empty? part, so maybe we could extract most of the method into a private method and redefine pluralize to this:

def pluralize(word)
  somethingize(word, inflections.plurals)
  # Alternatively:
  somethingize(word, :plurals) # (and just do inflections.send(:plurals) in somethingize)
end

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 6, 2011

Contributor

Agreed with somethingize(word, inflections.plurals). The word.empty? check could exist in both methods, it is just a very simple optimization.

@josevalim

josevalim Nov 6, 2011

Contributor

Agreed with somethingize(word, inflections.plurals). The word.empty? check could exist in both methods, it is just a very simple optimization.

This comment has been minimized.

Show comment Hide comment
@sarahhodne

sarahhodne Nov 6, 2011

Contributor

We would need a better name than somethingize though. Any suggestions?

@sarahhodne

sarahhodne Nov 6, 2011

Contributor

We would need a better name than somethingize though. Any suggestions?

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 6, 2011

Contributor

inflectionize? :P apply_inflections?

@josevalim

josevalim Nov 6, 2011

Contributor

inflectionize? :P apply_inflections?

sarahhodne added a commit to sarahhodne/rails that referenced this pull request Nov 6, 2011

@sarahhodne

This comment has been minimized.

Show comment Hide comment
@sarahhodne

sarahhodne Nov 6, 2011

Contributor

I added the change in pluralize to my commit in #3537, but the test change should maybe still be added?

Contributor

sarahhodne commented Nov 6, 2011

I added the change in pluralize to my commit in #3537, but the test change should maybe still be added?

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 6, 2011

Contributor

Yes, could you please rebase that pull request or send a new one with the test only? Thanks a lot!

Contributor

josevalim commented Nov 6, 2011

Yes, could you please rebase that pull request or send a new one with the test only? Thanks a lot!

@pksunkara

This comment has been minimized.

Show comment Hide comment
@pksunkara

pksunkara Nov 6, 2011

Contributor

Damn! I wanted to have a commit in rails. It would inspire me to contribute more.

Test case coming through.

Contributor

pksunkara commented Nov 6, 2011

Damn! I wanted to have a commit in rails. It would inspire me to contribute more.

Test case coming through.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 6, 2011

Contributor

@pkumar you still can! I am waiting on the test :)

Contributor

josevalim commented Nov 6, 2011

@pkumar you still can! I am waiting on the test :)

@pksunkara

This comment has been minimized.

Show comment Hide comment
@pksunkara

pksunkara Nov 6, 2011

Contributor

Done. Please merge.

Contributor

pksunkara commented Nov 6, 2011

Done. Please merge.

josevalim added a commit that referenced this pull request Nov 6, 2011

Merge pull request #3536 from pkumar/master
Make pluralize follow singularize's logic of uncountability check

@josevalim josevalim merged commit 6950639 into rails:master Nov 6, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment