Reference ancestor_hierarchies in depth instead of ancestors #398
Conversation
|
Sounds good, can you add a changelog entry ? |
|
@seuros long delay here :) but I added to changelog. Can we could get this pr merged? |
Tranquility
left a comment
Tranquility
left a comment
There was a problem hiding this comment.
Why do you even need the includes? ancestors is a single query that returns all the ancestors.
|
|
||
| def depth | ||
| ancestors.size | ||
| ancestor_hierarchies.size |
There was a problem hiding this comment.
This code actually changes the behavior ancestor_hierarchies includes the self and ancestors does not. So this increases the result by 1.
There was a problem hiding this comment.
should we just do ancestor_hierarchies - 1 ?
There was a problem hiding this comment.
I don't understand why this change is needed at all.
There was a problem hiding this comment.
This makes it so you can call depth on a node in a collection without an n+1 query (except it is off by 1 🤦).
Tag.includes(:ancestor_hierarchies).map(&:depth)
There was a problem hiding this comment.
@weilandia can you fix it and add a small test ? i will release a new version.
3 participants
This is helpful because we can more easily eager load ancestor_hierarchies than we can ancestors to avoid n + 1 queries.