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

Allow to configure custom inflector #591

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Allow to configure custom inflector #591

merged 1 commit into from
Jun 24, 2020

Conversation

flash-gordon
Copy link
Member

@flash-gordon flash-gordon commented Mar 1, 2020

All components that rely on inflector use an injected inflector now rather than using a global ROM::Inflector. This makes it possible to configure a custom inflector like that:

configuration = ROM::Configuration.new(:memory, "something") do |conf|
  conf.inflector = my_custom_inflector
end

@solnic
Copy link
Member

solnic commented Mar 2, 2020

@flash-gordon wouldn't it be better to just have ROM.config.inflector with a helper module for all the classes that needs it? It seems overkillish to inject it through options everywhere.

@flash-gordon
Copy link
Member Author

@solnic I don't know yet. It's kinda experiment, things like this allow to learn more about your code. Overall, I think inflector should be injectable. This may be not an issue for me personally but for Hanami user/developer relying on a global state is a well-known PITA. Code-wise it seems to be not that bad.

@solnic
Copy link
Member

solnic commented Mar 2, 2020

@flash-gordon yeah I hear you. It was just an immediate thought I had when I looked at the diff.

@flash-gordon flash-gordon marked this pull request as ready for review March 9, 2020 10:47
@flash-gordon flash-gordon requested a review from solnic March 9, 2020 10:47
@flash-gordon
Copy link
Member Author

@solnic apart from failing specs on 2.4 it's ready to go and I believe it's worth it. The changes are not radical and it brings some flexibility. Of course, it won't work when you instantiate two containers with different inflectors but the same relation classes but this case doesn't exist in the real world I'm sure.

@solnic solnic changed the base branch from master to release-5.2 June 23, 2020 06:22
@solnic solnic changed the base branch from release-5.2 to master June 23, 2020 06:22
@solnic
Copy link
Member

solnic commented Jun 23, 2020

@flash-gordon I'm sorry but I forgot about this PR and now we have a bunch of conflicts. Do you want this in 5.x or just 6.0?

@flash-gordon
Copy link
Member Author

@solnic np, I'll rebase today, it's for 6.0

@flash-gordon
Copy link
Member Author

done

@solnic solnic force-pushed the custom-inflector branch 3 times, most recently from 3e6a3fc to a51556b Compare June 24, 2020 08:58
@solnic solnic force-pushed the custom-inflector branch from a51556b to ef44993 Compare June 24, 2020 09:05
@solnic solnic added the feature label Jun 24, 2020
@solnic solnic added this to the 6.0.0 milestone Jun 24, 2020
@solnic solnic merged commit 35fd28a into master Jun 24, 2020
@solnic solnic deleted the custom-inflector branch June 24, 2020 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants