[Idea] Remove InstanceMethods mixin

[kbrock]

Documenting an idea for changing the way ancestry is mixed into the code.
Instead of including a static set of classes that use class level variables, it would dynamically define those methods using class level helpers and passing in all values of interest.

This basically follows the way that rails defines attributes and associations.

This will:

• remove all the send(ancestry_column) calls (which I do worry are currently a little broken)
• Possibly remove the use of ancestry_base_class.
• Allow multiple ancestry trees in a single table
• Possibly allow more flexibility in the definition of hierarchy implementations.

Before

module Ancestry
  module InstanceMethods
    def ancestor_ids
      self.class.parse_ancestry(self.send(@@ancestry_column))
    end
  end
  module ClassMethods
    def has_ancestry
      @@ancestry_delimiter = '/'
      @@ancestry_column = 'ancestry'
      include Ancestry::InstanceMethods
    end
    def parse_ancestry(string)
      string.split(@@ancestry_delimiter)
    end
  end
end
ActiveRecord::Base.include Ancestry::ClassMethods

class Model < ActiveRecord::Base
  has_ancestry
end

After

• would use Rail's CodeGenerator like ActiveModel::AttributeMethods#define_proxy_call
• Could still expose delimiter and the ancestry column, but no longer use send or base_class.send.
• define_ancestry_instance_methods probably lives in MaterializedPath
• Thinking that dynamic method definition are faster than all the send(ancestry_column). Since rails has been tuning this for years, using their method seems like a safe bet and it will leverage any speedups they make in the future. (need to verify)

module Ancestry
  module ClassMethods
    def has_ancestry
      # local variables
      ancestry_column  = 'ancestry'
      ancestry_delimiter = '/'
      define_ancestry_method(ancestry_column, ancestry_delimiter)
    end
    def define_ancestry_methods(ancestry_column, ancestry_delimiter)
      # if we still need these
      class_eval("def ancestry_column ; '#{ancestry_column}' ; end")
      class_eval("def ancestor_ids ; self.class.pars_ancestry(#{ancestry_column}, '#{ancestry_delimiter}') ; end")
    end
    def parse(ancestry_string, ancestry_delimiter)
      ancestry_string.split(ancestry_delimiter)
    end
  end
end
ActiveRecord::Base.include Ancestry::ClassMethods

class Model < ActiveRecord::Base
  has_ancestry
end

[kshnurov]

You want to dynamically define ~60 instance methods we currently have? Do you realize how terrible it's gonna look?
All of this sounds like a complete refactoring of the whole code, which will make it almost unreadable, and I don't quite get the motivation behind it. Are you really willing to do it?

multiple ancestry trees

As I said here: if you need multiple ancestries (do you?), you should definitely use another approach to ancestry - separate table (like closure_tree or your own implementation). It will be much faster and convenient to use.

remove all the send(ancestry_column) calls (which I do worry are currently a little broken)

What exactly is broken? Have you looked inside ActiveModel::AttributeMethods#define_proxy_call? All it does is send.

Thinking that dynamic method definition are faster than all the send(ancestry_column). Since rails has been tuning this for years, using their method seems like a safe bet and it will leverage any speedups they make in the future. (need to verify)

How's that? With ~60 dynamic definitions you'll lose much more than you'll gain from removing a few sends.

[kbrock]

Well, this was on my mind and I wanted to document it.

You want to dynamically define ~60 instance methods we currently have?

You are right. That is a lot.

Do you realize how terrible it's gonna look? All of this sounds like a complete refactoring of the whole code, which will make it almost unreadable.

I do wonder about this, I think they could look similar enough:

def subtree depth_options = {}
  self.ancestry_base_class.ordered_by_ancestry.scope_depth(depth_options, depth).subtree_of(self)
end

define_ancestry_method("subtree", "depth_options={}") do
  "self.class.order_by_ancestry(#{ancestry_column}).scope_depth(depth_options, depth).subtree_of(self)"
end

This would be a pain to debug. (debugging rails code is often a pain with basic methods)

I also think the methods could be defined in a way that allows for extensibility like updates to column caches and the like.

Are you really willing to do it?

Probably not.

As a first step, I probably need to see if I could get the ancestry column out of the class methods in the first place.

multiple ancestry trees
if you need multiple ancestries (do you?)

Agreed. I do have one case where I could use this, and our solution has not been the best but we are not in a hurry to change it. It is confusing and not necessary.

[kbrock]

update:

• Currently removing global variables from the class methods that are not needed.
  Not jumping through any hurdles, just the low hanging fruit.
  It seems that there are quite a few concepts that do not need to reside in class level variables.
• has_ancestry.rb defines dynamic scopes (e.g.: before_depth). I'm asking where it makes sense to leverage this kind of code instead of using class variables.
• Also, these scopes are probably holding onto a closure, and this method has a lot of state in it. I'll need to test the memory consumption, but it may make sense to move this into a separate method with fewer variables so the closure has less data in it.

[kbrock]

remove all the send(ancestry_column) calls (which I do worry are currently a little broken)
What exactly is broken?

A user has submitted bugs when using an ancestry column other than ancestry.
Our test coverage for different ancestry columns is basically non-existent.
I will see how hard it is to update the tests to change the name of the ancestry column.