Skip to content
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

Implement DiffSyncModelFlags.NATURAL_DELETION_ORDER. #220

Merged
merged 2 commits into from
May 3, 2023

Conversation

Kircheneer
Copy link
Collaborator

Closes #97. Implement DiffSyncModelFlags.NATURAL_DELETION_ORDER which handles DiffSyncAction.DELETE elements differently by first deleting their children recursively before handling their parents.

Open points

  • Do we need a global flag for this? If somebody wants this as the default behavior, which is quite a reasonable request to me, they would need to supply this flag over and over again

@Kircheneer Kircheneer force-pushed the lk-natural-deletion-order branch 2 times, most recently from 456670e to 3e79a6a Compare March 19, 2023 10:28
@Kircheneer Kircheneer added this to the 1.8.0 milestone Mar 19, 2023
@Kircheneer Kircheneer mentioned this pull request Mar 19, 2023
@Kircheneer Kircheneer force-pushed the lk-natural-deletion-order branch from 3e79a6a to 58f4806 Compare March 19, 2023 11:41
Copy link
Collaborator

@chadell chadell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me looks good, but I would make it the default (and sole) behavior

@Kircheneer
Copy link
Collaborator Author

to me looks good, but I would make it the default (and sole) behavior

so @chadell your recommendation would be to drop this into 2.0 and make a backwards-incompatible change, changing the flag to be the default?

@chadell
Copy link
Collaborator

chadell commented Mar 20, 2023

to me looks good, but I would make it the default (and sole) behavior

so @chadell your recommendation would be to drop this into 2.0 and make a backwards-incompatible change, changing the flag to be the default?

as I see it, this seems more a bugfix than a feature as this deletion mode is the one I would expect by default. Actually, I don't see the other use case. So, my approach would be to do not even define a flag for it.
For the version, I am not sure it deserves a major bump, but if you want to be on the safe side, I'm not against it.

@itdependsnetworks
Copy link
Contributor

In agreement with Christian, it seemed odd to me to have this as a flag. In my mind, it should be handled the same way that order children is handled. https://diffsync.readthedocs.io/en/latest/core_engine/02-customize-diff-class.html?#change-the-order-in-which-the-element-are-being-processed

@Kircheneer
Copy link
Collaborator Author

In agreement with Christian, it seemed odd to me to have this as a flag. In my mind, it should be handled the same way that order children is handled. https://diffsync.readthedocs.io/en/latest/core_engine/02-customize-diff-class.html?#change-the-order-in-which-the-element-are-being-processed

Unfortunately you can't achieve this behaviour from this MR in a custom Diff class - this is because the Diff class can only change the order of top level models, it never sees their children. The children (from the perspective of the Diff class) are handled via their respective parents.

diffsync/enum.py Show resolved Hide resolved
docs/source/core_engine/01-flags.md Show resolved Hide resolved
@Kircheneer
Copy link
Collaborator Author

My plan is now:

  1. Release in 1.8 as a flag
  2. Reverse flag (unnatural deletion order) for 2.0
  3. When it comes to 3.0, check if anyone is using the reversed flag for unnatural deletion order

@Kircheneer Kircheneer merged commit 2967c2e into develop May 3, 2023
@chadell chadell deleted the lk-natural-deletion-order branch May 4, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DiffSyncModel: Create/Update ordering and Delete ordering
4 participants