[Idea] Remove InstanceMethods mixin
Closed this issue · 4 comments
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
orbase_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
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 send
s.
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.
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.
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.