Add Inflection test (and fixes) to ensure singularizing a singular actually give you the correct singular in more cases #4719

Merged
merged 1 commit into from Feb 10, 2012

6 participants

@markmcspadden

I have a singularize issue, but in running through the some previous singularize issues tonight (#2608 #1825 #2395) I do understand that the current Core position is not to touch the inflections. I get it...but there is a major point release on the horizon, so now is as good a time as any....

I started out just wanting to address the case of singularizing words that end in 'ss'. In the current state "dress".singularize becomes "dres" and "boss" becomes "bos", etc. I think this is beyond just an edge case and should be handled by the framework.

When I got into the code, I realized there was a section for testing that pluralized words returned the expected pluralized versions, but not the same for singular. I added the test for "test_singularize_singular_#{singular}" and to my surprise adding this set of tests caused 13 failures. This may or may not be a surprise to you guys...and there may be a good reason why this set of tests isn't in there.

But in the case that there is not, I made corrections to make that set of tests pass and in the process my double s problem was fixed. (The words 'process' and 'address' were already in the test case and failed when the new set of tests was added.)

I'm open to suggestions on the changes themselves, but do believe that it's a reasonable expectation that the cases tested for work in this case. Thanks!

@josevalim
Ruby on Rails member
@jeffreyiacono
Ruby on Rails member

This commit breaks the action pack tests, specifically:

ruby -I lib:test test/dispatch/routing_test.rb -n test_resources_are_not_pluralized

/cc @fxn

Ruby on Rails member

I see the culprit is the ax in

inflect.singular(/(cris|ax|test)(is|es)$/i, '\1is')

Working on it.

Thanks @fxn

(totally prepared for a "this is why inflections are frozen" to follow)

Ruby on Rails member

Haha.

Ruby on Rails member

The good news is that the regression uncovered a bogus test in AP, fixed everything in fd5d9b3.

Thanks for hunting it down and fixing it up.

@bokmann

Really nice. I've been bitten by this too. Its nice that the test you added (singularizing singular names) has an existing parallel test in pluralizing pluralized names... makes it seem like this was definitely an oversight.

@markmcspadden

@josevalim - Thanks for passing it along.

@fxn and/or @jeremy - Any thoughts? (I know it's a very very small piece of Rails, but don't want to let it get forgotten.)

@markmcspadden

Weekly bump. Still interested in hearing from @fxn or @jeremy as to whether adjusting inflections to better accommodate the singularization of singular words is going to make it in.

@fxn
Ruby on Rails member

@markmcspadden thanks for the ping, I'll try to reply shortly.

@fxn
Ruby on Rails member

As you say the inflector is considered to be frozen. But I am positive about this pull request:

  • First and most important: the loop in the test suite was missing. I think that needs to be fixed, and if in order for the test to pass we need to fix some inflections, so be it.
  • We are going to apply this to master, so heading a major 4.0.

Let's see what does @jeremy think as well.

@fxn
Ruby on Rails member

We have also thumbs up from @jeremy. In then, thanks very much!

@fxn fxn merged commit 6728191 into rails:master Feb 10, 2012
@chancancode chancancode added a commit to chancancode/rails that referenced this pull request Jun 21, 2012
@chancancode chancancode Adds missing inflector tests to ensure idempotency
This is a follow up to #4719. It appears that singularize and pluralize
are supposed to be idempotent - i.e. when you call singularize or
pluralize multiple times on the same string, you should get the same
result. (At least for the "officially supported" cases that the stock
inflector is designed to handle.) #4719 added the missing tests for
regular cases, and this commit added the missing tests for the
irregularities.

While I'm at that, I also synced up the irregularity test cases with
the current set of irregularity cases that we ship out-of-the-box.
fe933be
@senny senny added a commit to senny/rails that referenced this pull request Mar 6, 2013
@senny senny CHANGELOG entry for improved singularizing of singulars.
Closes #9559.

The actual patch was added with #4719
f8b0e54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment