dry-rb / dry-inflector

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

Instantiated inflector objects are not configurable #44

Open swilgosz opened 1 year ago

swilgosz commented 1 year ago

Describe the bug

The current way to configure inflector object is as follows:

require "dry/inflector"

inflector = Dry::Inflector.new do |inflections|
  inflections.plural      "virus",   "viruses" # specify a rule for #pluralize
  inflections.singular    "thieves", "thief"   # specify a rule for #singularize
  inflections.uncountable "dry-inflector"      # add an exception for an uncountable word
end

This has some caveats though

  1. Inflections is a private method on inflector instance. 2.plural/singular/acronym methods are called on inflections.

this makes it troublesome to extend pre-confiured inflections for existing libraries like ROM in example: https://github.com/rom-rb/rom/blob/release-5.2/core/lib/rom/support/inflector.rb

To extend exiting Inflector, we need to either override the existing inflector duplicating it's rules, or access inflections private methods - both ways seem to be weird.

To Reproduce

inflector -= Dry::Inflector.new
inflector.inflections.plural "virus", "viruses"
image

Expected behavior

I expect to have a way of extending exiting inflector.

  1. Either by allowing inflections as public methods
  2. By passing existing inflector as an argument to method like: Dry::Inflector.extend(existing_inflector) do |inflections| - that could copy existing inflection rules.

Then I expect documentation to clearly describe how to aproach such situations, to avoid producing Ruby gems that are

I have some concerns against just making inflections public, as I very much like to see inflector object as an injectable, frozen dependency that people cannot change at runtime.

My environment

Note

PS: I'm happy to come up with a PR once we'll clarify the preferrable solution.

Ptico commented 1 year ago

Hi, previous year me and Piotr had a conversation about removing the default inflections and having a #merge method similar to what you suggest (https://discourse.dry-rb.org/t/dry-inflector-defaults/1422/2). I do have an initial implementation, it just needs some cleanup. The question is, as I didn't make it before 1.0 release, should we postpone the defaults removing?

swilgosz commented 1 year ago

I would start from extending functionality with backward compatibility fully kept.

The other idea from solnic could be, if we make inflections public, to have a #finalize method that would freeze the object from further modifications.

Ptico commented 1 year ago

I don't like an idea of having an explicit finalize method in this case as it makes the configuration pretty unpredictable when used inside other framework and force having an additional step for standalone usage

swilgosz commented 1 year ago

Agree. The merge method returning new instance without affecting old object state is more what I would go with, and in ROM 6 inflector becomes injectable so part of my problem would be gone.

Then we would need to yet tweak docs a little bit to include the best practices for gems

solnic commented 1 year ago

This wouldn't be a regular way of using it but in a framework reality, where it controls what happens, this makes sense. An inflector could be exposed to anything that may need it so that external components could configure it according to their requirements, and then the framework would finalize it to prevent further changes. This is how it works in case of dry-configurable and we already went through the "don't allow changing it" phase with this gem and it was quite problematic and inconvenient.

swilgosz commented 1 year ago

Can we then set consensus, that the expected solution would be:

  1. Expose inflections to public
  2. Add #finalize method that freezes the object (no more changes allowed)
  3. Tweak the docs to explain it clearly?
Ptico commented 1 year ago

I still heavily don't like #finalize method as it is making a wrong expression of necessity, therefore it can (and will) be used unreasonably early (like dry-configurable in a dry-system container by default), which may reduce flexibility in a configuration. While I had an experiments on transition tables compilations, which may require real finalization, right now we only need this to explicitly forbid later changes and I know one standard method with the same purpose, which is, you know, #freeze. So if we are going to expose inflection, may be it worth to just make proper freezing?

solnic commented 1 year ago

We can just use freeze yes. The most important thing is to allow setting up inflections outside of the inflector block. I don't remember why we ended up with finalize in dry-configurable btw.

Drowze commented 1 year ago

Hey guys 👋 I have a similar problem to the one described on this issue - hope I'm not hijacking the discussion :-)

While migrating a project to Hanami 2 I faced the issue with constants like:

# app/processors/api/foobar.rb
MyProject::Processors::Api::Foobar

Zeitwerk would accuse an error like:

NameError: uninitialized constant MyProject::Processors::Api::Foobar
Did you mean?  MyProject::Processors::API::Foobar

I understand that API is a sensible default for acronyms, but I did not find a way to remove that acronym - so I'm actually renaming my constants. Having the possibility to ignore or overwrite the defaults would be the best case for me :-) so I'm definitely looking forward to a change here!

Ptico commented 1 year ago

@solnic Sorry for pinging you so often, but I just realized that we may add an option to not load default inflections besides the very basic once, like:

inflector = Dry::Inflector.new(load_defaults: false) do |inflections|
htcarr3 commented 1 year ago

Any update on this? I am a fan of the load_defaults flag, and am currently running into this issue.