Make the apply_inflections method case-sensitive #14247

Merged
merged 1 commit into from Jun 26, 2014

Conversation

Projects
None yet
4 participants
Member

robin850 commented Mar 2, 2014

Hello,

Since d3071db, the apply_inflections method check if the downcased version of a string is contained inside the "whitelist" of uncountable words. However, if the word is composed of capital letters, it won't be
matched in the list while it should.

We can't simply revert to the previous behavior as there is a performance concern (benchmarked over /usr/share/dict/words):

user system total real
Before d3071db 135.610000 0.290000 135.900000 (137.807081)
Since d3071db 22.170000 0.020000 22.190000 ( 22.530005)
With the patch 25.130000 0.070000 25.200000 ( 25.611538)

Benchmarked with http://git.io/aFnWig.

Have a nice day.

Member

arthurnn commented Mar 3, 2014

👍

Member

robin850 commented May 3, 2014

@fxn : Would you mind having a look please ? :-)

@rafaelfranca rafaelfranca and 2 others commented on an outdated diff May 4, 2014

activesupport/CHANGELOG.md
@@ -1,3 +1,8 @@
+* Make the `apply_inflections` method case-sensitive when checking
@rafaelfranca

rafaelfranca May 4, 2014

Owner

Should not it be case-insensitive now?

@robin850

robin850 May 6, 2014

Member

I may be wrong but I think "case-sensitive" is right. At the moment, if we want to make the "HTTP" string uncountable, we'd have to write inflections.uncountable "http" (that would apply to "Http", "HtTp", etc. too) so I'd say that this is case-insensitive currently.

With this patch, half the problem is resolved. Actually if we write inflections.uncountable "HTTP", "HTTP" is defined as an uncountable but "http", "Http", etc. too. There is no easy way to fix this as people may except "Information".pluralize to return "Information" if we have the inflections.uncountable "information rule.

@matthewd

matthewd May 7, 2014

Owner

I think it's now case insensitive in both directions; previously it was insensitive to the case of the string being inflected, but very sensitive (as in, only one worked) to the case when the inflection is defined.

Owner

matthewd commented May 7, 2014

We don't want to just downcase the incoming string at the time the inflection is defined?

Owner

rafaelfranca commented May 19, 2014

@matthewd if we do that HTTP and HTTp will be the same and I think they should not.

Owner

matthewd commented May 19, 2014

Actually if we write inflections.uncountable "HTTP", "HTTP" is defined as an uncountable but "http", "Http", etc. too.

As I understand it, "HTTP" and "HTTp" are getting treated identically either way: there's only one place we use uncountables, and in that place, we're mapping it through downcase before we use the value. So why bother storing the correctly-cased version? 😕

Owner

rafaelfranca commented May 19, 2014

Yes, make sense. I think we should store the downcase version

Member

robin850 commented Jun 1, 2014

@rafaelfranca @matthewd : Your comments have been addressed normally, thanks for your feedback and sorry it took so long to get a response from my side! :-)

I've updated the commit message to reflect the actual performances ; this seems even faster with your suggestion (we are meeting the same performances as it is currently on master).

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Jun 26, 2014

...vesupport/lib/active_support/inflector/inflections.rb
@@ -160,7 +160,7 @@ def irregular(singular, plural)
# uncountable 'money', 'information'
# uncountable %w( money information rice )
def uncountable(*words)
- (@uncountables << words).flatten!
+ (@uncountables << words.flatten.map(&:downcase)).flatten!
@rafaelfranca

rafaelfranca Jun 26, 2014

Owner

Can we use map!?

@rafaelfranca

rafaelfranca Jun 26, 2014

Owner

And since we already call flatten before the map we still need the flatten!?

@robin850

robin850 Jun 26, 2014

Member

As for #map!, I'm not really sure. People may pass an array built from a file or the database so it'd be altered but I don't know if this is that common. What do you think ?

As for the two flatten calls, I agree that they don't fit here. Actually this was here because we can pass arrays to the uncoutable method (e.g. here) but the words parameter is still an array so we ended up in having an array of arrays. Another implementation could be:

@uncountables += words.flatten.map(&:downcase)

Which is also a bit faster.

@robin850 robin850 Make the apply_inflections method case-sensitive
Since d3071db, the apply_inflections method check if the downcased
version of a string is contained inside the "whitelist" of uncountable
words. However, if the word is composed of capital letters, it won't be
matched in the list while it should.

We can't simply revert to the previous behavior as there is a
performance concern (benchmarked over /usr/share/dict/words):

Before d3071db  135.610000   0.290000  135.900000 (137.807081)
Since d3071db   22.170000    0.020000  22.190000  ( 22.530005)
With the patch   22.060000    0.020000  22.080000  ( 22.125771)

Benchmarked with http://git.io/aFnWig

This way, the solution is to put the down-case version of words inside
the @uncountables array.
643409d

@rafaelfranca rafaelfranca added a commit that referenced this pull request Jun 26, 2014

@rafaelfranca rafaelfranca Merge pull request #14247 from robin850/inflections-with-uncountables
Make the apply_inflections method case-sensitive
d04763f

@rafaelfranca rafaelfranca merged commit d04763f into rails:master Jun 26, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

robin850 deleted the robin850:inflections-with-uncountables branch Jun 27, 2014

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