WIP: Allow to pass a proc as fallback for conditional fallbacks

[doits]

I'm trying to make fallbacks more dynamic (which is in the same vein as #314 for reference). There I did a default workaround to make this possible, but I think it should be implemented directly in fallbacks module. To recap:

I have a User model and each user can select his own main_locale which should be used as a fallback for all translations.

This means that fallbacks are different per instance and not application or class wide anymore. To make this possible I updated the fallbacks plugin to allow a proc as a option:

translates :biography, fallbacks: proc { main_locale }

The proc gets passed the model as a first argument.

No arguments get passed at the moment, not sure if anything should be passed from the plugin.

---

ToDo:

• Add new specs
  • done for 0-8-stable
  • done for master
• See if old specs got broken and fix them
  • done for 0-8-stable
  • done for master
• Maybe refactor it a little bit to be less spaghetti-like
• Add it to master branch, too

---

Another possible solution I can think of is to pass a database column or sql query to fetch the locale.

• Pro: Fallbacks could be used when querying/sorting/... then, too (but this could get complicated fast for each backend I assume)
• Con: It is much less flexible than a proc and requires to have fallbacks saved in the database exactly like they should be used (maybe this is a pro?)

---

What do you generally say about this idea? Is this something you'd want to have directly in the fallbacks plugin? Do you see any side effects?

If this looks reasonable to integrate I can add the specs and refactor it it.

[shioyama]

Thanks very much for the PR! I have to think about this a bit, but also there seems to be a test failing with a "stack level too deep". This looks like it might not be related to the changes here. Could you try checking that the specs pass without the change? I'll try creating a build from master to see if it passes by default or if something changed in Rails.

The fallbacks plugin code, btw, is quite confusing right now with fallback and fallbacks... wondering if an API change might be called for in v1.0.

[shioyama]

Ok created #329. If it passes then the code here is breaking them.

[doits]

I have to think about this a bit [...]

Take your time, looking for the best solution for this problem and you are the expert :-)

Could you try checking that the specs pass without the change? I'll try creating a build from master to see if it passes by default or if something changed in Rails.

The PR is against 0-8-stable branch, maybe this branch is broken. Didn't run specs locally though yet, will have to when I write new ones at the latest though :-)

The fallbacks plugin code, btw, is quite confusing right now with fallback and fallbacks... wondering if an API change might be called for in v1.0.

Yeah, I'd say this is a good one to clean up. Make it the same everywhere (I'd vote for singular version) and accept the same options with the same behavior for everything but true: In config and translates call it means to use default I18n fallbacks, on instance level call (foo.title(fallback: true)) it means to use the configured fallbacks (same as omitting fallback option).

[shioyama]

Hmm ok well #329 passes. The PR should be against master, but 0-8-stable should be passing too.

[doits]

Yeah, it is on the ToDo list to have it on master, too ... I wanted to have something to use fast, that's why I went with 0-8-stable (since trying out mobility master in my project broke it, but I didn't investigate long why and switched back to 0-8-stable).

[shioyama]

Ah that's right I started making many changes toward v1.0 on master, which explains why things were maybe breaking for you. I'll check if tests are passing without changes on 0-8-stable.

[shioyama]

Indeed specs are failing on 0-8-stable, I'll look into it...

[doits]

Yeah, though here more specs are failing ... just updated the branch to see if this fixes those spec failures with a subtle logic change.

[doits]

I think I fixed the spec errors by refactoring the code a little bit. In addition I added some specs for the new functionality.

This should pass on 0-8-stable now (except the unrelated failing specs in #331).

I can port this to master if you feel it is worth it, possible refactoring the option's name ( fallback everywhere), just ping me.

[doits]

Yeah, now all old spec pass and I like the latest commit sha 59fffff ;-)

[doits]

@shioyama any thoughts on this in the meantime? Is this something for 1.x, or do you have an idea how to implement this differently? Or not a feature you want to have in here?

[shioyama]

@doits Thanks very much, I see specs are passing which is good, and I see that you've added considerable test coverage. However the change adds many lines to the plugin... so I'm a bit hesitant. I agree that the feature itself would probably be attractive to many people, but ideally I'd like to see if we could simplify the implementation further.

I need a bit of time to look at the code carefully, unfortunately I don't really have that time now and priority is on #323 right now. I'll see if I can make some suggestions inline in the meantime so maybe we can improve the PR.

But again, thanks very much for the work and thought, it's a good idea and I'm not against adding it if we can find a way to make it a bit simpler, or more encapsulated perhaps.

[shioyama]

I should add that the requirements for the fallbacks plugin are already too convoluted, which is why you had to write all those ugly conditionals to handle things like fallback: false. It might not be a bad idea at this point to target v1.0 and change the API for fallbacks to make things simpler.

To do that, I think the right place to start would be to ask what is a reasonable expectation for a fallbacks API. The current plugin was kind of patched together from APIs of similar gems (Globalize, etc.) and from user requests (fallback: false).

[shioyama]

Also, although perhaps tests were failing there, I'd prefer to have this as a PR on the master branch because this will not get merged into 0.8.x, it'll be 0.9 or 1.0 if it's merged. If tests are failing it would be great to know which ones.

[doits]

I agree that the feature itself would probably be attractive to many people [...]

That's what I wanted to know, good to hear it 👍

[...] but ideally I'd like to see if we could simplify the implementation further.

Yes, I feel like this, too.

I need a bit of time to look at the code carefully, unfortunately I don't really have that time now and priority is on #323 right now. I'll see if I can make some suggestions inline in the meantime so maybe we can improve the PR.

#323 is what bites me currently, too, so go on with that one first.

To do that, I think the right place to start would be to ask what is a reasonable expectation for a fallbacks API. The current plugin was kind of patched together from APIs of similar gems (Globalize, etc.) and from user requests (fallback: false).

I can try to think of a good API with the same functionality for master, if you want to, and refactor it that way, already having in mind this feature but implement the new API without it first (so there is only the new API without a change of functionality first).

Then this feature could be added in a separate MR on top of the new API, which is probably much simpler then.

So the plan would be to:

1. MR1: Make the fallback API simpler on master without breaking existing or adding new functionality (targeted for next major release, probably 1.0)
2. MR2: Add this feature on top of the new fallback API (maybe in 1.0 or 1.1, depending on how fast 1.0 is released after MR1 is ready)

I can start with 1. if you like to, by first thinking of a new api and describing it (before implementing it). Of course if you already thought about it or have some expectations, I'd like to know them.

[shioyama]

@doits Very sorry for taking a while to reply, 1. sounds good if you want to make that PR.

[doits]

👍 Just that you know I'm currently really busy but still have this on my todo list ... might take some time for me though until I can think about this again.

[shioyama]

@doits No problem! I've finally gotten around to fixing the issue in #323, once that's released I'm going to try to finally get v1.0 released. Although I've said this many times and it never seems to happen... 🤣

[doits]

Time passes by and ... I just saw you released 1.0.0 🎉 congrats, great work @shioyama! And with the Plugin architecture I could easily implement this feature as a separate plugin like this:

Edit: See #328 (comment) for an updated version.

module FallbackProcPlugin
  extend ::Mobility::Plugin

  requires :backend, include: :before

  included_hook do |_, backend_class|
    backend_class.include BackendReader.new(options[:fallback_proc])
  end

  class BackendReader < Module
    def initialize(fallback_proc)
      define_method :read do |locale, **options|
        value = super(locale, **options)

        return value if !fallback_proc || value.present?

        fallback_locale = model.instance_exec(&fallback_proc)

        return value if fallback_locale.blank?

        super(fallback_locale, **options)
      end
    end
  end

  ::Mobility::Plugins.register_plugin(:fallback_proc, FallbackProcPlugin)
end

Mobility.configure do
  plugins do
    fallback_proc
  end
end

class SomeModel
  extend Mobility

  attribute :main_locale

  translates :title, fallback_proc: proc {
    # return any locale you want from here

    # fallback to `main_locale` attribute of current model
    main_locale
  }
end

If you think this is something for Mobility core functionality, I can clean it up (maybe rename it?) and add it in a separate PR with some specs. Setting a default proc could be implemented, too Edit: Works out of the box.

Otherwise I am happy I could write my own plugin and it works so easily :-)

---

Edit: Setting a default fallback proc is also possible but requires some work around by defining the proc before as a variable:

Mobility.configure do
  default_fallback_proc = proc { ... }

  plugins do
    fallback_proc default_fallback_proc
  end
end

Without the variable declaration above plugins do the delegation tries to load the plugin proc, which is wrong here and unexpected. But it works like this.

[shioyama]

Thanks @doits ! Great that you've already picked up the new plugin format 👍

Now that 1.0 has been released I'd be interested in improving the original fallbacks plugin to include this feature and also simplify the other existing options. We could then to aim to release that change in a v1.1.

Let me think about this a bit more but if anyone else is following has thoughts on what a "fallbacks" plugin should have in it please share here 😄

[doits]

Maybe having multiple simple fallback plugins is the way to go? Something like:

• static_fallbacks --> just a rename from fallbacks, like the current one in Mobility (to not break backwards compatibility)
• model_attribute_fallbacks --> Look at a specific model attribute for a fallback locale, so no need to pass a proc (might be faster?)
• dynamic_fallbacks --> pass a proc like above (WIP: Allow to pass a proc as fallback for conditional fallbacks #328 (comment)) and let it be completely dynamic, it can do whatever it wants to find a fallback locale

This could keep each plugin simple, but of course users would then need to choose which one is the right for them.

[shioyama]

@doits A few things quickly:

• I think this is a sensible addition, so I'd like to include it in an upcoming 1.1 release. I'm not merging anything to 0-8 anymore and anything new will only apply to versions >= 1.0 (so any PR should be to master).
• I think it's ok to support procs in the default plugin, since it would just mean that in place of fallbacks or boolean as the option value, you would pass a block. i.e. translates :foo, fallbacks: ->(locale) { ... }. I think this would be fairly intuitive.
• That said, the current internals of the fallbacks plugin are ugly, to some degree because the requirements on the API are too complex.

Regarding the last point, I simplified things slightly by removing new_fallbacks and other methods from the (former) Mobility::Configuration class (which is now gone in 1.0). But there is still some fairly tricky stuff in that plugin. Part of this complexity is because of I18n, and maybe unavoidable.

I'm going to create a quick branch to see what this would look like on master.

[shioyama]

I think first thing to clarify is usage (regardless of implementation). I was imagining that you would pass a proc which would take a locale, and return the fallback locale. Maybe the self there could be the model instance, not really sure.

If anyone else is following, what would be the most natural thing in this case?

[doits]

I was imagining that you would pass a proc which would take a locale, and return the fallback locale

Yeah, the locale probably should be passed, too, in case the fallback depends on the original locale.

Maybe the self there could be the model instance, not really sure

I think this is reasonable. Since the idea is to have a fallback specific for an instance it makes sense to make accessing the instance as easy as possible.

---

For reference, this is what I am using in my app for many months without problems already (though the original locale is not passed to the proc here yet, but this is a simple addition):

# last updated 2021-10-04

# Fallback locale is returned by some attribute or method of the instance
# 
# To use the locale in column `main_locale` as fallback locale:
# translates :title, model_attribute_fallback: :main_locale
#
# In this example `main_locale` can be a model attribute or even a *method* with some custom logic
module ModelAttributeFallbackPlugin
  extend ::Mobility::Plugin

  requires :backend, include: :before

  included_hook do |_, backend_class|
    next unless options[:model_attribute_fallback]

    backend_class.include BackendReader.new(options[:model_attribute_fallback])
  end

  class BackendReader < Module
    def initialize(fallback_attribute)
      define_method :read do |locale, **options|
        value = super(locale, **options)

        if value.present? ||
           options[:locale] ||
           options[:fallback] == false ||
           options[:model_attribute_fallback] == false
          return value
        end

        fallback_locale = model.public_send(fallback_attribute)

        return value if fallback_locale.blank? || fallback_locale == locale

        super(fallback_locale, fallback: false, **options)
      end
    end
  end

  ::Mobility::Plugins.register_plugin(:model_attribute_fallback, ModelAttributeFallbackPlugin)
end

# Fallback locale is returned by a proc, `self` is the model instance when proc is called
# 
# To use:
# translates :title, fallback_proc: proc { ...custom_logic, `self` is the model instance... }
module FallbackProcPlugin
  extend ::Mobility::Plugin

  requires :backend, include: :before

  included_hook do |_, backend_class|
    next unless options[:fallback_proc]

    backend_class.include BackendReader.new(options[:fallback_proc])
  end

  class BackendReader < Module
    def initialize(fallback_proc)
      define_method :read do |locale, **options|
        value = super(locale, **options)

        if value.present? ||
           options[:locale] ||
           options[:fallback] == false ||
           options[:fallback_proc] == false
          return value
        end

        fallback_locale = model.instance_exec(&fallback_proc)

        return value if fallback_locale.blank? || fallback_locale == locale

        super(fallback_locale, fallback: false, **options)
      end
    end
  end

  ::Mobility::Plugins.register_plugin(:fallback_proc, FallbackProcPlugin)
end

[shioyama]

@doits Really glad you're making use of the plugin framework. I do feel though that there's some unneeded complexity, and I'd like to make it easier to do what you're doing (e.g. without having to use define_method etc).

#538 should do that.

Your example above could then be simplified given the fact that options is accessible from inside the backend:

module ModelAttributeFallbackPlugin
  extend ::Mobility::Plugin

  requires :backend, include: :before

  module BackendMethods
    def read(locale, **kwargs)
      return super unless (fallback_attribute = options[:model_attribute_fallback])

      value = super

      if value.present? ||
        kwargs[:locale] ||
        kwargs[:fallback] == false ||
        kwargs[:model_attribute_fallback] == false
        return value
      end

      fallback_locale = model.public_send(fallback_attribute)

      return value if fallback_locale.blank? || fallback_locale == locale

      super(fallback_locale, fallback: false, **kwargs)
    end
  end

  ::Mobility::Plugins.register_plugin(:model_attribute_fallback, ModelAttributeFallbackPlugin)
end

module FallbackProcPlugin
  extend ::Mobility::Plugin

  requires :backend, include: :before

  module BackendMethods
    def read(locale, **kwargs)
      return super unless (fallback_proc = options[:fallback_proc])
 
      value = super

      if value.present? ||
         kwargs[:locale] ||
         kwargs[:fallback] == false ||
         kwargs[:fallback_proc] == false
        return value
      end

      fallback_locale = model.instance_exec(&fallback_proc)

      return value if fallback_locale.blank? || fallback_locale == locale

      super(fallback_locale, fallback: false, **kwargs)
    end
  end

  ::Mobility::Plugins.register_plugin(:fallback_proc, FallbackProcPlugin)
end

I'm not following the context much here and haven't actually tested the changes, just modified them to demonstrate how things can be simplified. Basically you shouldn't need a module builder or an included_hook (after #538 is merged).

Notice I'm distinguishing options from kwargs, the latter are what are passed to the read/write methods, the former are options on the translation class (for plugins). I'm going to try to adopt this strictly throughout the codebase because the term "options" is getting confusing as it's currently used. Hope this makes things a bit clearer.