Siblings shouldn't return self

[justinperkins]

In the docs (and how the gem behaves), it states that the siblings scope will return all child nodes of the node's parent. Effectively the same as doing: node.parent.children

When you ask for siblings, you aren't asking for yourself. To achieve this we have to write it like this: node.siblings.where.not(id: node.id).

Not the worst thing in the world, but from a semantics perspective it feels definitively wrong.

[kbrock]

Performance wise, siblings is much simpler to program all of my parent's children. In many of the cases, this distinction doesn't matter to the code so taking this hit does not make sense.

While I have heard people say "we are all siblings", referring to themselves, I tend to think that "my siblings" does not include myself. I do hear people use "other siblings" to be clear.

I do worry about changing the definition of siblings - even in a major release.

Do you like the idea of other_siblings to signify this concept?
Do you want to throw together a pr for that method?

[justinperkins]

I think other_siblings perpetuates confusing terminology. The common definition of siblings is one or more brothers and/or sisters of yourself. Including yourself in that list is strange.

Agree that making a change to siblings should be approached cautiously.

[kbrock]

So then there would be a configuration on changing the definition for siblings?

Not sure how to go about transitioning this and not screwing the existing users up

[justinperkins]

Yeah it'd be a breaking change for sure. I'd deprecate it if I owned this gem. I absolutely love it other than this minor inconvenience. Beats recursive DB calls all day long

[jtoy]

just ran into this myself, I vote for deprecate.

[kbrock]

Maybe create a global configuration setting to change it.
Unfortunately we would have to default to the old behavior

Or add a gem post_install_message to configure settings.
In that message we may want to suggest different encoding patterns.

[kbrock]

For now, you can use siblings.exclude(self).

It seems like it would have a universal convention around this, but I haven't seen it.

I did check closure_trees and they have self_and_siblings and siblings.
I'm trying to remember the other gem that returns self along with siblings.

It has been this way for so long I'm hesitant to change it.

I have started to introduce global settings.
At some time in the future I may add feature switches for some of these behaviors, like siblings.

[justinperkins]

It has been this way for so long I'm hesitant to change it.

Yup, I'm with you. Maybe just called out in the README since it feels like an unexpected anti-pattern.

Feature switches could be a nice way to transition away from it.