New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the default "f" => "ve" plural inflection. #6820

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@davidcelis
Copy link
Contributor

davidcelis commented Jun 21, 2012

This patch is to remove a default from ActiveSupport's inflections. The
current default of words ending in "fe" to be pluralized as "ves" is
strange in that this is a grammatical exception rather than the rule.
The conversion of "f" to "ve" in pluralization is not as common as the
case of simply adding the "s" and, when the exception does occur, should
we really care? I believe it can end up causing more confusion than anything
else. Aside from words like "half" (which was covered in the same rule
but with a different regex case), or the words "life" (and perhaps "knife" for Chef
users), I am unsure as to why people would need to pluralize these words. I
would be open to discussion on what other words should be added to the list
of irregulars, but when is the last time you saw has_many :wives?

Signed-off-by: David Celis david@davidcelis.com

David Celis
Remove the default "fe" => "ves" plural inflection.
This patch is to remove a default from ActiveSupport's inflections. The
current default of words ending in "fe" to be pluralized as "ves" is
strange in that this is a grammatical exception rather than the rule.
The conversion of "f" to "ve" in pluralization is not as common as the
case of simply adding the "s" and, when the exception does occur, should
we really care? Aside from "half" (which was covered in the same rule
but with a different regex case), "life", and perhaps "knife" for Chef
users, I am unsure as to why people would need to pluralize these words.
When is the last time you saw `has_many :wives`, for instance?

Signed-off-by: David Celis <david@davidcelis.com>
@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented Jun 21, 2012

Thanks for your pull request, but technically inflections are frozen in Rails, meaning that they're not going to receive changes anymore because this can easily break existing applications.

In any case, I'll ping @fxn to review this first. Thanks!

@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented Jun 21, 2012

There are definitely legitimate use cases for those. "3 lives remaining"?

@davidcelis

This comment has been minimized.

Copy link
Contributor Author

davidcelis commented Jun 21, 2012

@carlosantoniodasilva Yeah, I saw that this is a "long standing policy" with Rails but figured I'd open the PR anyway. This just strikes me as an incredibly weird default. It would break existing applications but people should not blindly upgrade Rails without checking the change log. Perhaps it should not be a 3.2 candidate for that reason, but I appreciate you not closing this immediately regardless.

@chancancode If you look at the actual code, I included "life" as an irregularity as it is a frequently used word in certain areas of software development. But other words that follow this exception, such as "wife", are not so common.

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented Jun 21, 2012

@davidcelis alright, thanks for contributing. Lets wait for more feedback from others :)

@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented Jun 21, 2012

@davidcelis That debate actually comes up a lot, see #1825, #2395, #3055 and many more. The resolution has almost always been "won't fix" (if we fix this, what about X and Y and Z?). However, if you want to increase the odds of a merge, try doing something like #5177 :)

@davidcelis

This comment has been minimized.

Copy link
Contributor Author

davidcelis commented Jun 21, 2012

@chancancode Thanks for the suggestion:

% egrep '([aeiou])fe$' /usr/share/dict/words
afterlife
agrufe
alewife
Algarsife
Aoife
applewife
broadwife
bushwife
butterwife
carafe
cauldrife
chafe
cocklewife
counterlife
dehnstufe
drawknife
duckwife
elfwife
enlife
Fife
fife
firesafe
fishwife
flaxwife
goodwife
gudewife
henwife
herbwife
hostlerwife
housewife
ife
jackknife
kalewife
kitchenwife
knife
life
loosestrife
matchsafe
midwife
nife
nonlife
oldwife
ottrelife
overchafe
overrife
oysterwife
penknife
piewife
pocketknife
puddingwife
Rafe
rechafe
rife
roupingwife
safe
saltwife
seawife
shopwife
sife
spaewife
stife
strafe
strife
supersafe
Tartufe
tripewife
underlife
understrife
unrife
unsafe
vouchsafe
walkrife
wanrufe
washerwife
wastrife
waukrife
wife
wildlife
witchwife
Yellowknife

% egrep 'ves$' /usr/share/dict/words
Aves
avives
bagleaves
bedstaves
behooves
calves
consecutives
dyeleaves
eaves
elves
fives
Graves
greaves
halves
hives
Itaves
leaves
ourselves
pelves
theirselves
themselves
vives
yourselves

Out of this first list, I only see variants of "life, "knife", "wife" to be correct with the current rule. Other words (agrufe, carafe, fife, rafe, rife, safe, sife, stife, strafe, strife, etc.) are all incorrect (you can see a fuller list with egrep '([^lwn][aeiou])fe$' /usr/share/dict/words)

Out of the second list, aves, fives, graves, greaves, pelves, and vives are all wrong (hives was marked as an irregular). Words that don't appear on that list for some reason, but would also be wrong, are drives and reprieves (probably others that I can't think of immediately). It seems pretty clear to me that this default rule is more of an exception and should be handled as an exception on individual cases.

@epochwolf

This comment has been minimized.

Copy link

epochwolf commented Jun 21, 2012

+1: The plural of safe is not saves, it's safes

@iGEL

This comment has been minimized.

Copy link
Contributor

iGEL commented Jun 24, 2012

Maybe all pluralization rules should be pulled out of rails into the initializer, so they can be updated without breaking existing apps. To provide an upgrade path, Rails should refuse to start if no rules are configured, returning the url to a gist which you can copy & paste to get the Rails 3.2 rules.

@davidcelis

This comment has been minimized.

Copy link
Contributor Author

davidcelis commented Jun 24, 2012

I'm not sure if that's the correct route to go; to me, that would seem to be going against the Rails philosophy of convention over configuration. I think it's fine that Rails provides pluralization rules, but they should be the actual rules rather than exceptions to the rules.

@iGEL

This comment has been minimized.

Copy link
Contributor

iGEL commented Jun 24, 2012

Well, for most users there would be no difference, if the rules are implemented within the active-support gem or in a generated initializer.

@davidcelis

This comment has been minimized.

Copy link
Contributor Author

davidcelis commented Jun 24, 2012

Sure, I suppose it's easy enough to change the inflections from an initializer; it may make sense to define them in the initializer as well considering it's always generated. If that would allow Rails to unfreeze their inflection rules, I'd be for it and willing to update this patch to make that change.

@davidcelis

This comment has been minimized.

Copy link
Contributor Author

davidcelis commented Jul 7, 2012

Two week bump.

davidcelis pushed a commit to davidcelis/rails that referenced this pull request Jul 16, 2012

David Celis
Move ActiveSupport inflections to the initializer template
This is a patch meant for 4.0 that will move most inflections that are
currently defined in `activesupport/lib/active_support/inflections.rb`
to the inflection initializer template defined in Railties.

Inflections are currently "frozen" in Rails. Commits that add/remove/alter
inflections are largely ignored whether or not they make sense when
considering English grammar. The freezing was a decision made to avoid
breaking existing apps that depend on these defined inflections. By moving the
inflections to an initializer, we can unfreeze this area of Rails and change
inflections without fear of breaking existing applications.

Additionally, this will garner more understanding as to how Rails
pluralization/singularization works. For oddities that occur in the
inflections Rails has defined, it's often simply a "WTF?" moment and
people have to source dive in order to figure out why their
singularization/pluralization is not working as expected. Laying them
all out in the initializer will reduce a lot of headache associated with
inflection oddities.

Two things to note:

1. I've left the two basic inflections within ActiveSupport. These are:
  * Add an "s" to make a word plural (unless it already has one)
  * Remove an s to make a word singular (unless it ends in two)

  I'm not actually sure about whether that second one should be
  included, or even if I should move both to the initializer as well.
  I'd appreciate some input on that.

2. I have another pull request open (pull request rails#6820) that would need
   to be modified if this is merged.

Signed-off-by: David Celis <david@davidcelis.com>
@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Sep 16, 2012

The inflector is frozen, and it's not worth breaking existing apps to make these kinds of changes. I'm giving this a close. If @fxn disagrees he can re-open.

@hahahana

This comment has been minimized.

Copy link

hahahana commented Jan 17, 2013

I know this issue is closed, but I ran across another issue along the same vein, although it is a little more involved than changing the default inflections. When you generate a scaffold with a name that ends in "ve", in a few cases the controller will have the instance variables pluralized than singularized again, without regard to the original singular scaffold name. So for example, if I generate a scaffold named Agave the controller will generate this:

class AgavesController < ApplicationController
  # GET /agaves
  # GET /agaves.json
  def index
    @agaves = Agave.all

    respond_to do |format|
      format.html # index.html.erb
      format.json { render json: @agaves }
    end
  end

  # GET /agaves/1
  # GET /agaves/1.json
  def show
    @agafe = Agave.find(params[:id])

    respond_to do |format|
      format.html # show.html.erb
      format.json { render json: @agafe }
    end
  end

So even though I gave the name Agave to the generator, the given name was ignored while creating the instance variables for agaves#show (and update, destroy, etc.), resulting in "agafe" instead of "agave".

I can just go ahead and change the inflections file (inflect.irregular 'agave', 'agaves') and then generate the scaffold, but I think this is in general probably not the desired behavior for the scaffold. Changing the default behavior of "ve"-> "f" would fix this particular issue, but there are probably other cases where pluralizing for the controller than singularizing for the instance variables could also be a gotcha.

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