-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extensible orphan_strategy #658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Just a couple of suggestions there :D
if method_defined?(orphan_strategy_helper) | ||
alias_method :apply_orphan_strategy, orphan_strategy_helper | ||
before_destroy :apply_orphan_strategy | ||
elsif orphan_strategy.to_s != "none" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just compare with the symbol to avoid calling to_s
?
README.md
Outdated
:orphan_strategy Instruct Ancestry what to do with children of a node that is destroyed: | ||
:destroy All children are destroyed as well (default) | ||
:rootify The children of the destroyed node become root nodes | ||
:restrict An AncestryException is raised if any children exist | ||
:adopt The orphan subtree is added to the parent of the deleted node | ||
If the deleted node is Root, then rootify the orphan subtree | ||
:none skip this logic. (add your own `before_destroy`) | ||
String Custom. `before_destroy :apply_orphan_strategy_#{String}` to handle orphans. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might need a little expansion so that people know they don't need to add their own before_destroy
block here. Perhaps:
Your custom destroy strategy method named with the convention apply_orphan_strategy_#{String}
will be executed as an before_destroy
callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, After trying to document this had me wanting to drop the feature apply_orphan_strategy_#{string}
and just have the person write a before_destroy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that makes sense :)
3b3edca
to
b01c78e
Compare
b01c78e
to
967445d
Compare
Ok, so this stalled. I think that introducing
|
967445d
to
562fd6f
Compare
also tried for some consistency in messaging around attributes
562fd6f
to
c0d2bf6
Compare
update:
|
Many people override apply_orphan_strategy and either leave it blank or call custom methods from there. Introducing `orphan_strategy: :none` to allows developers to skip default orphan handling. From here a developer can add `before_destroy` with what ever logic is desired. It is possible to introduce a custom orphan strategy with a mixin/concern, but it doesn't seem to save any code and is not the best interface with too many nuances. Leaving in the code for now but not promoting to a supported feature yet.
c0d2bf6
to
753013a
Compare
update:
|
Looks good :) When I'm upgrading next, I'll give it a go. It might be that the custom orphan strategy would need to be defined immediately after |
Actually, the custom method needs to be defined before the But in looking through github, the overrides for this method were almost all to disable it. |
I think in my case it was the order of deletion but I think that was also fixed recently so I might not need the custom method anyway. I've not started moving to Rails 7 yet but when I do, this gem will get upgraded :D |
@brendon Thanks. That info helps me. Someone asked about when this will ship. I'm probably going to backport this, as I am making (slow) progress towards converting many functions to sql only. |
This introduces 2 ways for developers to extend the built in orphan strategies.
Fixes #142
/cc @brendon Here is custom orphan strategies.
After implementing this, I'm thinking of keeping
:none
and dropping the custom string.none
is straightforward and simple to document. The custom one is much harder (red flag)before_destroy
seems simple.apply_orphan_strategy
, but maybe it is not so bad.As a developer, what is your take?
Turning orphan strategy handling off
Implementing a custom orphan strategy