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

Memoization stopped working on upgrading to 1.0.0 #128

Closed
abime opened this issue Nov 26, 2021 · 7 comments
Closed

Memoization stopped working on upgrading to 1.0.0 #128

abime opened this issue Nov 26, 2021 · 7 comments

Comments

@abime
Copy link
Contributor

abime commented Nov 26, 2021

We found that Memoization stopped working on upgrading to 1.0.0.

We believe that the issue got introduced in this #124, where Implementation module got removed and it affected the ancestor chain.

Initially lookup method from Memoize module used to take the precedence but after removal of Implementation module lookup method of I18n::Backend::ActiveRecord is called.

Ancestor Chain in 0.4.1

[I18n::Backend::ActiveRecord, I18n::Backend::Memoize, I18n::Backend::ActiveRecord::Implementation, I18n::Backend::Base, I18n::Backend::Transliterator, I18n::Backend::Flatten, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, ActiveAdminHelper, PP::ObjectMixin, ERB::Util, Delayed::MessageSending, ActiveSupport::Dependencies::Loadable, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject]

Ancestor Chain in 1.0.0

[I18n::Backend::ActiveRecord, I18n::Backend::Memoize, I18n::Backend::Flatten, I18n::Backend::Base, I18n::Backend::Transliterator, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, ActiveAdminHelper, PP::ObjectMixin, ERB::Util, Delayed::MessageSending, ActiveSupport::Dependencies::Loadable, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject]

Two possible solutions to make memoization work

  1. Use prepend instead of include in the initializer. If we consider taking this approach, we need to make changes to the readme and the initializer generator code.
I18n::Backend::ActiveRecord.send(:prepend, I18n::Backend::Memoize)
  1. Revert removal of Implementation Module.

Let us know what you think and we can raise the PR accordingly.

Co-Authored by @prayeshshah

@timfjord
Copy link
Collaborator

Thanks for reporting the issue.

I'd go with the first solution and update the README.
Feel free to open a PR otherwise I will do this next week

@abime
Copy link
Contributor Author

abime commented Nov 29, 2021

Hey @timfjord

We have raised a PR for the first solution here #129

On skimming through code from other i18n backends(simple, key-value), we noticed that the they all have Implementation module present. And they follow the convention of including instead of prepending the Memoize module.

Should we also be in sync with other I18n backends and use Implementation module?

@timfjord
Copy link
Collaborator

Hey @abime

Thanks for sending the PR

Yeah, I've been thinking about this and I think are right, it is better to stick to the convention and bring the Implementation module back.
Sorry for the back and forth.
Please let me know if you wish you update #129 accordingly, otherwise, I will take care of that

@abime
Copy link
Contributor Author

abime commented Nov 29, 2021

@timfjord No problem. I will update #129 in sometime and send it for review.

@prayeshshah
Copy link
Contributor

@timfjord We've made the necessary changes and rebased the branch with master. Please re-review.

@timfjord
Copy link
Collaborator

Awesome, thanks @prayeshshah
Let's switch to the PR to discuss the implementation.

@timfjord
Copy link
Collaborator

timfjord commented Dec 1, 2021

Just released v1.1.0

Thanks for the contribution @prayeshshah and @abime

@timfjord timfjord closed this as completed Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants