Skip to content
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

Instantiated inflector objects are not configurable #44

Open
swilgosz opened this issue Mar 10, 2023 · 11 comments
Open

Instantiated inflector objects are not configurable #44

swilgosz opened this issue Mar 10, 2023 · 11 comments

Comments

@swilgosz
Copy link

swilgosz commented Mar 10, 2023

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

  • Affects my production application: YES (makes it troublesome to upgrade ROM)
  • Ruby version: 2.7.7
  • OS: Mac OS

Note

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

@Ptico
Copy link
Collaborator

Ptico commented Mar 10, 2023

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
Copy link
Author

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
Copy link
Collaborator

Ptico commented Mar 10, 2023

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
Copy link
Author

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
Copy link
Member

solnic commented Mar 12, 2023

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
Copy link
Author

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
Copy link
Collaborator

Ptico commented Mar 13, 2023

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
Copy link
Member

solnic commented Mar 14, 2023

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
Copy link

Drowze commented Mar 16, 2023

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
Copy link
Collaborator

Ptico commented Mar 16, 2023

@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
Copy link

htcarr3 commented Jul 23, 2023

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

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

No branches or pull requests

5 participants