Deprecate adding ancestry to the middle of an STI tree #626
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the helper function `with_model` calls has_ancestry for us So essentially we are calling this twice. - fixed to not define has_ancestry at the root node. This is now ensuring that search and the counter_caches are working.
Provide deprecation warning for has_ancestry in the middle of a tree Still haven't thought up with a good use case for this.
|
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 |
|
What's the purpose of this deprecation? Makes zero sense. |
|
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 I also need to remove the deprecation from 4-3-stable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is also being merged into 4.3.x branch via #625revertedAdded 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_ancestryin 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_ancestryat 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 #617 but it looks like that may be invasive. may want to backport that PR and tweak it