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

Disable setting default_options hash directly #92

Closed
shioyama opened this Issue Sep 14, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@shioyama
Owner

shioyama commented Sep 14, 2017

Context

Since 0.2.0, it has become possible to set Mobility.default_options which are merged into the options set whenever translates is called on a Mobility model. However, the problem with this is that the "default" default_options is overridden if the user sets these options with Mobility.default_options =, which will typically disable "low-profile" plugins like presence and cache.

Expected Behavior

Setting Mobility.default_options = raises an error and instructs the user to set options by their key, e.g.:

Mobility.configure do |config|
  # ...

  config.default_options[:fallbacks] = { ... }
  config.default_options[:dirty] = true
  # ...
end

Actual Behavior

Mobility allows default options to be overridden entirely, implicitly disabling any options which were previously true but now are nil.

Possible Fix

Add a deprecation warning on the setter in 0.3.0, and remove thereafter.

@shioyama shioyama changed the title from Disable setting default_options directly to Disable setting default_options hash directly Sep 14, 2017

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Sep 15, 2017

Collaborator

An alternative approach would be to configure the default behavior within the plugin itself, instead of having a default default_options.

For instance,

module Presence
  def self.apply(attributes, option)
    attributes.backend_class.include(self) if option != false
  end
end

In the previous example, if the user didn't specify anything, option would be nil, so the plugin would be enabled. If a user explicitly sets presence: false, then the plugin would be disabled.

The only downside to this approach is that given the current implementation, there is no way to tell if the user specified presence: nil. That's probably not a big deal, but if it is an issue, you could initialize the plugins like

Mobility.plugins.each do |name|
  plugin = get_plugin_class(name)
  plugin.apply(self, !options.has_key?(name), options[name])
end

and change the method signature of apply to

def self.apply(attributes, default, option)

One advantage of this approach is it keeps the default behaviour of a plugin contained within the plugin itself.

Collaborator

pwim commented Sep 15, 2017

An alternative approach would be to configure the default behavior within the plugin itself, instead of having a default default_options.

For instance,

module Presence
  def self.apply(attributes, option)
    attributes.backend_class.include(self) if option != false
  end
end

In the previous example, if the user didn't specify anything, option would be nil, so the plugin would be enabled. If a user explicitly sets presence: false, then the plugin would be disabled.

The only downside to this approach is that given the current implementation, there is no way to tell if the user specified presence: nil. That's probably not a big deal, but if it is an issue, you could initialize the plugins like

Mobility.plugins.each do |name|
  plugin = get_plugin_class(name)
  plugin.apply(self, !options.has_key?(name), options[name])
end

and change the method signature of apply to

def self.apply(attributes, default, option)

One advantage of this approach is it keeps the default behaviour of a plugin contained within the plugin itself.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 15, 2017

Owner

Yes I thought about that as one way to do it. The issue I have is that checking for false is not really a good pattern IMHO. The option should really be true or false.

One advantage of this approach is it keeps the default behaviour of a plugin contained within the plugin itself.

In a way, yes, but on the other hand it means looking at the default_options hash alone is not enough to get a clear idea of what is on by default and what is off. Whereas if true universally means "on", then just looking at the default options hash you get a clear idea of what is enabled and what is not.

So personally I prefer having a set of defaults in Mobility.default_options and have the ability to reset each one per-key.

That said, the plugins are not consistent in the sense of true/false above. I think actually fallbacks should be false by default (same as above, but for consistency).

Owner

shioyama commented Sep 15, 2017

Yes I thought about that as one way to do it. The issue I have is that checking for false is not really a good pattern IMHO. The option should really be true or false.

One advantage of this approach is it keeps the default behaviour of a plugin contained within the plugin itself.

In a way, yes, but on the other hand it means looking at the default_options hash alone is not enough to get a clear idea of what is on by default and what is off. Whereas if true universally means "on", then just looking at the default options hash you get a clear idea of what is enabled and what is not.

So personally I prefer having a set of defaults in Mobility.default_options and have the ability to reset each one per-key.

That said, the plugins are not consistent in the sense of true/false above. I think actually fallbacks should be false by default (same as above, but for consistency).

@shioyama shioyama added the chore label Sep 15, 2017

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Sep 15, 2017

Collaborator

I see your point. Raising visibility to what options are enabled by default is important. One other approach you probably also considered was having rails generate mobility:install just generate a config that has setup like

Mobility.configure do |config|
  config.default_options = {
    cache: true,
    dirty: false,
    fallbacks: nil,
    presence: true,
    default: nil
  }
end

That way, it would be quite obvious how to configure mobility.

The main downside to that approach is maintainability, as it makes it harder to change defaults per version, or guarantee that keys are set.

Collaborator

pwim commented Sep 15, 2017

I see your point. Raising visibility to what options are enabled by default is important. One other approach you probably also considered was having rails generate mobility:install just generate a config that has setup like

Mobility.configure do |config|
  config.default_options = {
    cache: true,
    dirty: false,
    fallbacks: nil,
    presence: true,
    default: nil
  }
end

That way, it would be quite obvious how to configure mobility.

The main downside to that approach is maintainability, as it makes it harder to change defaults per version, or guarantee that keys are set.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 15, 2017

Owner

Yes, that would be another way to do it. I'm worried like you said about maintainability though. If we use this approach, and in future release we have another plugin that should be on by default, we'll have the same problem again.

Owner

shioyama commented Sep 15, 2017

Yes, that would be another way to do it. I'm worried like you said about maintainability though. If we use this approach, and in future release we have another plugin that should be on by default, we'll have the same problem again.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 15, 2017

Owner

Actually, looking at the plugins again, I remember now why fallbacks is nil and not false.

The tricky thing is that with a plugin like fallbacks, you really have three different things you may want:

  • disable the plugin entirely so that it is not even included into the backend (in the case of fallbacks, this would mean that even post.title(fallback: :en) would not work, since no plugin would even receive the fallback: :en option to the reader)
  • disabled by default, but included (in this case, post.title will not apply fallbacks, but post.title(fallback: :en) would work using :en as fallback since the fallback plugin would be included.
  • enabled (post.title applies default fallbacks and post.title(fallback: :en) overrides the default fallbacks to use a different fallback locale)

You could argue that the first case (disable the plugin altogether) can also be achieved by removing fallbacks from Mobility.plugins, which is the array that Mobility iterates through when applying plugins:

Mobility.plugins.each do |name|
  plugin = get_plugin_class(name)
  plugin.apply(self, options[name])
end

However, this applies universally and you may just want to remove the fallbacks plugin from a particular model (say for debugging). So here false meaning something different from nil happens to be useful.

The default plugin also has a nil option value by default. This one is tricky since the option value is the default, so here the plugin is included regardless of the option value. To absolutely disable it (not even include it into the backend) you actually do need to remove it from the Mobility.plugins array, and thus remove it from all models.

So this whole options thing is slightly more complicated than I'd like it to be, but after thinking about this quite a lot (and simplifying things quite a lot from 0.1 to 0.2) this is the most straightforward setup I've managed to come up with. At least, the application of plugins is very simple (the four lines of code pasted above).

Sorry a bit of a digression... but if you have thoughts, interested to hear 😄

Owner

shioyama commented Sep 15, 2017

Actually, looking at the plugins again, I remember now why fallbacks is nil and not false.

The tricky thing is that with a plugin like fallbacks, you really have three different things you may want:

  • disable the plugin entirely so that it is not even included into the backend (in the case of fallbacks, this would mean that even post.title(fallback: :en) would not work, since no plugin would even receive the fallback: :en option to the reader)
  • disabled by default, but included (in this case, post.title will not apply fallbacks, but post.title(fallback: :en) would work using :en as fallback since the fallback plugin would be included.
  • enabled (post.title applies default fallbacks and post.title(fallback: :en) overrides the default fallbacks to use a different fallback locale)

You could argue that the first case (disable the plugin altogether) can also be achieved by removing fallbacks from Mobility.plugins, which is the array that Mobility iterates through when applying plugins:

Mobility.plugins.each do |name|
  plugin = get_plugin_class(name)
  plugin.apply(self, options[name])
end

However, this applies universally and you may just want to remove the fallbacks plugin from a particular model (say for debugging). So here false meaning something different from nil happens to be useful.

The default plugin also has a nil option value by default. This one is tricky since the option value is the default, so here the plugin is included regardless of the option value. To absolutely disable it (not even include it into the backend) you actually do need to remove it from the Mobility.plugins array, and thus remove it from all models.

So this whole options thing is slightly more complicated than I'd like it to be, but after thinking about this quite a lot (and simplifying things quite a lot from 0.1 to 0.2) this is the most straightforward setup I've managed to come up with. At least, the application of plugins is very simple (the four lines of code pasted above).

Sorry a bit of a digression... but if you have thoughts, interested to hear 😄

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