search

stefankroes / ancestry

Organise ActiveRecord model into a tree structure
MIT License
3.72k stars 458 forks source link

Deprecate adding ancestry to the middle of an STI tree #626

Closed kbrock closed 1 year ago

kbrock commented 1 year ago

This PR is also being merged into 4.3.x branch via #625 reverted

Added deprecation warnings for has_ancestry in the middle of an STI tree.

Still haven't thought up a good use case for why someone would define ancestry in the middle of an STI tree.

Due to the way it was introduce, it is possible that allowing has_ancestry in the middle of the tree was not an explicit goal but a side effect.

If we want to deprecate, need to put a warning into 4.3.x. Unsure if we will drop this functionality in 5.0 or 6.0.

The test helper has an implicit has_ancestry at the top level so it was masking the intent of the test. Whether we deprecate the feature or not, it is best to get this test into the code.

NOTE: as follow up, need to add deprecation for https://github.com/stefankroes/ancestry/pull/617 but it looks like that may be invasive. may want to backport that PR and tweak it

kbrock commented 1 year ago

as hashed out in #620 we will keep this column for now. We can revisit at a later time to see if having all the base_ancesty_class references buys us something over the standard rails base_class

kshnurov commented 1 year ago

What's the purpose of this deprecation? Makes zero sense.

kbrock commented 1 year ago

Agreed, I am putting together a PR that removes this deprecation and adds more tests on this case. Am waiting for 600 / 620 to go in.

I still wonder about the need for base_ancestry_class, but can address that at a later time.

I also need to remove the deprecation from 4-3-stable.