Add option for dynamic modules #67

Merged
merged 3 commits into from Mar 6, 2013

Conversation

Projects
None yet
3 participants
@dipth
Contributor

dipth commented Feb 22, 2013

Especially handy in a multi-tenancy setup where each tenant might not want the same modules and keys.

Add option for dynamic modules
Especially handy in a multi-tenancy setup where each tenant might not want the same modules and keys.
@dipth

This comment has been minimized.

Show comment
Hide comment
@dipth

dipth Feb 28, 2013

Contributor

@jkrall do you have time to take a look at this one?

Contributor

dipth commented Feb 28, 2013

@jkrall do you have time to take a look at this one?

@sfsekaran

This comment has been minimized.

Show comment
Hide comment
@sfsekaran

sfsekaran Feb 28, 2013

Collaborator

I'm still not decided on whether we should include this or not, but I think if we include this it could auto-detect whether we're passing in custom module configuration by checking if the modules are an array or a hash:

# original
analytical :modules => [:google]
# augmented
analytical :modules => { google: {...}, kissmetrics: {...} }
# or in your case:
analytical :modules => lambda{ |controller| controller.analytical_modules }

The point is I'd rather not add a separate configuration key for this if possible.

Collaborator

sfsekaran commented Feb 28, 2013

I'm still not decided on whether we should include this or not, but I think if we include this it could auto-detect whether we're passing in custom module configuration by checking if the modules are an array or a hash:

# original
analytical :modules => [:google]
# augmented
analytical :modules => { google: {...}, kissmetrics: {...} }
# or in your case:
analytical :modules => lambda{ |controller| controller.analytical_modules }

The point is I'd rather not add a separate configuration key for this if possible.

@dipth

This comment has been minimized.

Show comment
Hide comment
@dipth

dipth Mar 1, 2013

Contributor

@sfsekaran you mean something like this?
1122d2e

Contributor

dipth commented Mar 1, 2013

@sfsekaran you mean something like this?
1122d2e

Update README to reflect changes
and selflessly add myself to the list of contributors.
@sfsekaran

This comment has been minimized.

Show comment
Hide comment
@sfsekaran

sfsekaran Mar 4, 2013

Collaborator

Yes, much like that 👍

I'd like to get another maintainer's opinion before merging. @nirvdrum or @freerobby, any problems with this non-intrusive feature?

Collaborator

sfsekaran commented Mar 4, 2013

Yes, much like that 👍

I'd like to get another maintainer's opinion before merging. @nirvdrum or @freerobby, any problems with this non-intrusive feature?

@nirvdrum

This comment has been minimized.

Show comment
Hide comment
@nirvdrum

nirvdrum Mar 4, 2013

Collaborator

Overall, I really like this. I don't want to bikeshed, but it might be nice if the Proc variant had a way to fallback to whatever the config file has. Maybe if its yielded value is nil, you treat it as a no-op. E.g., it could be used to override config for just certain controllers or maybe just certain environments. But that could be done later as well.

Collaborator

nirvdrum commented Mar 4, 2013

Overall, I really like this. I don't want to bikeshed, but it might be nice if the Proc variant had a way to fallback to whatever the config file has. Maybe if its yielded value is nil, you treat it as a no-op. E.g., it could be used to override config for just certain controllers or maybe just certain environments. But that could be done later as well.

@dipth

This comment has been minimized.

Show comment
Hide comment
@dipth

dipth Mar 4, 2013

Contributor

@nirvdrum I like your idea but it might be a bit tricky to implement without changing the API or adding new config-keys.

The problem is that if we pass a modules-option to the call to analytical, the modules loaded from the yml-file never actually gets stored anywhere.

One way to do it would be like this:

  def analytical(options={})
    self.analytical_options = options

    config_options = { :modules => [], :file_modules => [] }
    File.open("#{::Rails.root}/config/analytical.yml") do |f|
      file_options = YAML::load(ERB.new(f.read).result).symbolize_keys
      env = (::Rails.env || :production).to_sym
      file_options = file_options[env] if file_options.has_key?(env)
      file_options.each do |k, v|
        if v.respond_to?(:symbolize_keys)
          # module configuration
          config_options[k.to_sym] = v.symbolize_keys
          config_options[:modules] << k.to_sym unless options && options[:modules]
          config_options[:file_modules] << k.to_sym
        else
          # regular option
          config_options[k.to_sym] = v
        end
      end if file_options
    end if File.exists?("#{::Rails.root}/config/analytical.yml")

    self.analytical_options = self.analytical_options.reverse_merge config_options
  end

Which makes sure that we can later see what modules were originally loaded from the yml-file, but then we are back at the original problem with introducing another key in the configuration hash

Contributor

dipth commented Mar 4, 2013

@nirvdrum I like your idea but it might be a bit tricky to implement without changing the API or adding new config-keys.

The problem is that if we pass a modules-option to the call to analytical, the modules loaded from the yml-file never actually gets stored anywhere.

One way to do it would be like this:

  def analytical(options={})
    self.analytical_options = options

    config_options = { :modules => [], :file_modules => [] }
    File.open("#{::Rails.root}/config/analytical.yml") do |f|
      file_options = YAML::load(ERB.new(f.read).result).symbolize_keys
      env = (::Rails.env || :production).to_sym
      file_options = file_options[env] if file_options.has_key?(env)
      file_options.each do |k, v|
        if v.respond_to?(:symbolize_keys)
          # module configuration
          config_options[k.to_sym] = v.symbolize_keys
          config_options[:modules] << k.to_sym unless options && options[:modules]
          config_options[:file_modules] << k.to_sym
        else
          # regular option
          config_options[k.to_sym] = v
        end
      end if file_options
    end if File.exists?("#{::Rails.root}/config/analytical.yml")

    self.analytical_options = self.analytical_options.reverse_merge config_options
  end

Which makes sure that we can later see what modules were originally loaded from the yml-file, but then we are back at the original problem with introducing another key in the configuration hash

@sfsekaran

This comment has been minimized.

Show comment
Hide comment
@sfsekaran

sfsekaran Mar 6, 2013

Collaborator

@nirvdrum, good point, and thanks @dipth for elaborating. Based on this, I'm going to merge this request and we can investigate good fallback support afterward. 🔨

Collaborator

sfsekaran commented Mar 6, 2013

@nirvdrum, good point, and thanks @dipth for elaborating. Based on this, I'm going to merge this request and we can investigate good fallback support afterward. 🔨

sfsekaran added a commit that referenced this pull request Mar 6, 2013

Merge pull request #67 from dipth/dynamic_modules
Add option for dynamic modules

@sfsekaran sfsekaran merged commit a0dc4f4 into jkrall:master Mar 6, 2013

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