Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  Daniel Schierbeck November 14th, 2008 @ 01:36 PM

  • Tag changed from “inflector, pluralize, uncountable” to “inflector, patch, pluralize, uncountable”

  When you write words = phrase.to_s.dup.split(" "), it seems that dup is redundant, and adds an unnecessary extra memory consumption. split is not destructive, and can safely be called on phrase.to_s.

• You flagged this item as spam.
  

  James Adam November 14th, 2008 @ 01:43 PM

  Updated patch to avoid using dup

  • fix_pluralize.patch 2.1 KB

• You flagged this item as spam.
  

  James Adam November 14th, 2008 @ 01:45 PM

  Great point Daniel

  I think the dup was only there as a hold-over from the previous implementation. I'll amend the patch now.

  Thanks again,

  James

  On 14 Nov 2008, at 13:36, Lighthouse wrote:

• You flagged this item as spam.
  

  Daniel Schierbeck November 14th, 2008 @ 02:20 PM

  +1, with the latest change, this looks like an elegant solution.

• 

  DHH November 16th, 2008 @ 03:13 PM

  Hmm, pluralize(word) intends to me that it's about pluralizing a word, not a phrase. If this was to pass, all the inflectors should be handling phrases. Which I think perhaps is a bit much to promise?

• You flagged this item as spam.
  

  James Adam November 16th, 2008 @ 07:54 PM

  That could be a fair point; part of the motivation behind the patch is that, by the examples in the RDoc above it, I do expect pluralize to work on phrases:

  
  "the blue mailman".pluralize # => "the blue mailmen"
  

  If that example were to be removed, I wouldn't have so readily expected pluralise to work on my phrase ("sports equipment"), and would've created a wrapper for pluralize in my own app.

  I'm not sure all the inflectors would need to know/care about phrases - probably only pluralize and singularize. As a rough sketch:

  
  def with_last_word(phrase, &block)
    words = phrase.split(" ")
    last_word = words.pop
    words.push(yield(last_word))
    words.join(" ")
  end
  
  def pluralize(phrase)
    with_last_word(phrase) { |w| w + 's' }
  end
  
  def singularize(phrase)
    with_last_word(phrase) { |w| w.gsub(/s$/, '') }
  end
  
  pluralize("the dancing fool") # => "the dancing fools"
  singularize("prancing idiots") # => "prancing idiot"
  

  If you feel this is worthwhile, I'll work up a proper patch for both methods. Otherwise, I feel that the 'blue mailmen' example should be removed, as it's misleading and encourages the broken behaviour.

• 

  DHH November 16th, 2008 @ 08:28 PM

  • State changed from “new” to “resolved”

  I agree. I think it's cleaner to keep pluralize/singularize to words as all the other inflectors do that too. I'll remove blue mailmen. Don't know how that snuck in there.