Make pluralize follow singularize's logic of uncountability check #3536
Conversation
| @@ -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 } | |||
There was a problem hiding this comment.
Thanks for the pull request. Shouldn't we move this logic to a private method and have both pluralize and singularize calling it?
There was a problem hiding this comment.
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)
endThere was a problem hiding this comment.
Agreed with somethingize(word, inflections.plurals). The word.empty? check could exist in both methods, it is just a very simple optimization.
There was a problem hiding this comment.
We would need a better name than somethingize though. Any suggestions?
There was a problem hiding this comment.
inflectionize? :P apply_inflections?
See diff discussion on rails/rails#3536.
|
I added the change in |
|
Yes, could you please rebase that pull request or send a new one with the test only? Thanks a lot! |
|
Damn! I wanted to have a commit in rails. It would inspire me to contribute more. Test case coming through. |
|
@pkumar you still can! I am waiting on the test :) |
|
Done. Please merge. |
Make pluralize follow singularize's logic of uncountability check
I have added a test case which fails prior to the patch. All the tests pass including the new one now.