dry-rb / dry-inflector

Inflector for Ruby
https://dry-rb.org/gems/dry-inflector
MIT License
95 stars 15 forks source link

Ensure pluralization of irregular words to be idempotent #10

Closed jodosha closed 6 years ago

jodosha commented 6 years ago

Tentatively closes #7

It makes pluralization of irregular words idempotent:

require "dry/inflector"

Dry::Inflector.new.pluralize("people") # => "people"

It's a backward compatibility fix for inflecto v0.0.2.

⚠️ Please read this ⚠️

While porting inflecto to dry-inflector, I looked at master branch of inflecto. I noticed a difference between v0.0.2 and master in terms of how irregular rules are added. This is the commit: https://github.com/mbj/inflecto/commit/05468ea24208e5cbae9290545a9b620b215b487d#diff-3f76f8e104060c974809c409ab7727d3

I assume that the code before this commit (probably taken from ActiveSupport::Inflector) was accidentally making the pluralization of irregular words idempotent. ("people" => "people").

Indeed, if you use master version of inflecto (at a39d9a5):

irb(main):001:0> Inflecto.pluralize("people")
=> "peoples"

/cc @mbj

abinoam commented 6 years ago

Hi @jodosha,

Congrats for tracing it.

If you have added this

plural(Regexp.new("(#{singular[0, 1]})#{singular[1..-1]}$", "i"), '\1' + plural[1..-1])
plural(Regexp.new("(#{plural[0, 1]})#{plural[1..-1]}$", "i"), '\1' + plural[1..-1])
singular(Regexp.new("(#{plural[0, 1]})#{plural[1..-1]}$", "i"), '\1' + singular[1..-1])

You're probably willing to also add

singular(Regexp.new("(#{singular[0, 1]})#{singular[1..-1]}$", "i"), '\1' + singular[1..-1])

If not ...

require "dry/inflector"

inflector = Dry::Inflector.new

inflector.pluralize("crisis") #=> "crises"
inflector.pluralize("crises") #=>  "crises"

inflector.singularize("crises") #=> "crisis"
inflector.singularize("crisis") #=> "crisi" <- FAIL

Well, but I'm still not completely convinced we should try to catch all bad users' entries.

jodosha commented 6 years ago

@abinoam

Well, but I'm still not completely convinced we should try to catch all bad users' entries.

I agree.

abinoam commented 6 years ago

What about dropping this PR and issue #7? We could let just the added specs. I liked them. Adjusting them to the expected behaviour without the idempotent related code.

But this would let us with a big problem for people trying to drop old inflecto and even active_support. Even rom-rb as @solnic stated. They'll expect the same behaviour of active_support, no matter if it's erratic or not.

We could solve this by providing a reasonable set of defaults. Then one could initialize its own inflector instance and use it in a per context basis.

It could be something like: (taking into account @jodosha suggestions at https://github.com/dry-rb/dry-inflector/issues/9#issuecomment-344598129)

# We could provide this

Dry::Inflector::Inflections::ActiveRecord = Dry::Inflector::Inflections.new do |inflection|
  inflection.plural(/people\z/i, "people")
end

# Then people could begin their code as:

inflector = Dry::Inflector.new(inflections: Dry::Inflector::Inflections::ActiveRecord)

I think we almost getting there. Would this solve the compatibility problem for rom-rb (eg: people -> people) @solnic ?

abinoam commented 6 years ago

I'll try to write some code so we can discuss it better.

jodosha commented 6 years ago

@abinoam Thanks for wrapping this. I completely agree with you.


One thing isn't clear to me:

We could let just the added specs. I liked them. Adjusting them to the expected behaviour without the idempotent related code.

How can we keep the current specs without implementing this feature? I mean, they won't pass without the corresponding changes in production code.

abinoam commented 6 years ago

It's more of a cosmetic change. Split the fixtures. Split regular from irregular. You have already done this.

jodosha commented 6 years ago

Closing this because we won't try to be "smart" and "fix" the input. This isn't a blocker anymore for ROM: https://github.com/dry-rb/dry-inflector/issues/7#issuecomment-345168791