Closed kbrock closed 1 year ago
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:
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.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.
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:
send(ancestry_column)
calls (which I do worry are currently a little broken)ancestry_base_class
.Before
After
send
orbase_class.send
.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)