[Discussion] Drop ancestry_base_class

[kbrock]

active_record defines base_class as the top of an STI tree.

ancestry had a similar concept. It was the class where ancestry was defined. Since ancestry originally called it a base_class, there was a conflict and in 4e57f32e1758 it was renamed to ancestry_base_class

PRs of note:

• 86

• 87

• e3ea26a54e1f6 (introduction of STI)

The ancestry_base_class is used quite often to make sure the proper class is accessed to lookup values.

I'm not sure the use case for defining ancestry in the middle of a hierarchy, but it doesn't seem to work in a reasonable manner. (UPDATE: the tests were faulty. I'm fixing the tests so it is properly tested)

class  Class1 < ActiveRecord:Base
end
class Class2 < Class1
  has_ancestry counter_cache: :child_count
end

root = Class2.create!
node = root.children.create!

root.children == []
node.root == root
root.child_count == 0

I'm dropping this custom concept and using the standard rails concept instead, as I think it should have been done originally.

---

Contemplating raising an error if a developer adds has_ancestry to a non base_class.

[kbrock]

/cc @Fryguy could you take a peek.

With these changes, manageiq-core along with relationships work fine.

Can you think of negative implications of changing this? Also, can you think of a reason why someone may even want to define has_ancestry on a child class and not the parent/base class? (We only define on the base class)

[Fryguy]

I can't really think of a valid reason. The only thing that comes to mind, and it feels way outside the norm, is if you want different has_ancestry settings for different subclasses from the same base class, but then I'd think you'd need multiple separate ancestry columns as well.

[kbrock]

@kshnurov do you have an opinion on this?

[kbrock]

will introduce some of the other #600 changes and then revisit this.

I'm on the fence but leaning towards yes.

[Fryguy]

This is a breaking change, right, so major version bump? (Or should we deprecate?)

[kshnurov]

@kshnurov do you have an opinion on this?

You're introducing a breaking change that adds/saves/solves nothing, but will break someone's use cases. What's the purpose, just to break something?

[kbrock]

@kshnurov do you have an opinion on this?

You're introducing a breaking change that adds/saves/solves nothing, but will break someone's use cases.

A simple, "Lets keep this. there is an issue that states it is needed" would suffice.

The phrase "I'm on the fence but leaning towards yes" means that I am unsure. That is why I reached out.

What's the purpose, just to break something?

Don't be rude just to be rude. This adds nothing. This is not the way to encourage me to ask for your input in the future.

Using something that rails has built in seemed to make sense over implementing something that is very close. And as I stated before, the mature in which this feature was introduced had me wondering if this feature was even intended behavior.

[kshnurov]

A simple, "Lets keep this. there is an issue that states it is needed" would suffice.

You mentioned that issue in the PR description! I didn't expect you didn't even read it 🤦🏻‍♂️

Don't be rude just to be rude. This adds nothing. This is not the way to encourage me to ask for your input in the future.

You've thrown out my opinion, proposals and work a few times already without any objective reasoning. I consider that being rude, so the attitude is the result of that and a few new bad PRs that make this gem only worse.

[kbrock]

I'll revisit this later

[kbrock]

For my own reference

• base_class was around in rails 1.1.6 (2006)
• base_class was introduced in ancestry 1.1.2 (2009) in e3ea26a54e1f
  • This said to add STI support (not specific whether it is adding STI in the middle)
  • confused why base_class was overridden instead of leveraging existing column
  • was this name intentionally picked, or was it a coincidence
• s/base_class/ancestry_base_class/g in #86
  • fixes conflicts with other gems
  • reason for replacing rather than just dropping the assignment of base_class?
  • fixes 87 as well. was this intentional?
• test to ensure mid STI support introduced in 4e57f32 fixed 87 : 0cc1c0f0
  • first reference in the code base that sti in the middle is a thing.

I guess I am stuck trying to figure out why someone would define an STI tree and only want to use part of it for an ancestry tree. Maybe there is a default scope in a portion of it?

Also stuck having a concept that seems to overlap with rails so closely. (especially for a feature that I do not think is valid. would prefer to not have extra concepts in the code)

Also concerned that the wrong class variables will be used or one will trample another. Put in a test to cover this (at least a little bit)

---

I am only ever going to define this at the root level. So whether there is this variable does not affect me. And if that premise is followed, it is not in the way for having multiple ancestry columns.