masciugo
commented
9 years ago Hi eric
It seems to me a good implementation but I have few things to mention before going to much further.
First of all I wouldn't add the path feature, for the moment at least. Besides being too specific, the output of the method would be inconsistent with other query methods and couldn't be chained. We can think later, if necessary, a way to have it optional.
As you can see, your code is not Rails3 compatible. I use this gem in a Rails3 project but maybe it's time to remove this compatibility..
maybe it's better to keep the method name consistent with PedHunter as we refer to it naming it _lowest_commonancestors. (plural as it, in general, returns ActiveRecord::Relation). Moreover, I think this method should be a class method of the genealozied model and should take 2+ individuals as argument and than aliased someway by instance methods:
MyModel.lowest_common_ancestors(manuel,luis,peter)
manuel.lowest_common_ancestors_with(luis,peter)
what do you think?
Lastly I'm going to add some little notes directly on your changes
estensland
I added the start of complex query methods from PedHunter, a basic implementation of finding least common ancestors.
I am not sure it is the best way, but it is functional. If you think it should be done better, I can do some more research. It currently only returns the ancestor, but not the path from the individuals to the least common ancestors as discussed on PedHunter. Once the path is discovered, this can also return how two individuals are related..such as "grandchild", "second-cousin", "first cousin once removed", etc.
I you would rather wait on this request until I can create a system that returns not only the ancestor but also at least the path between the two people, I can do that no problem.