search

networktocode / diffsync

A utility library for comparing and synchronizing different datasets.
https://diffsync.readthedocs.io/
Other
155 stars 26 forks source link

Implement DiffSyncModelFlags.NATURAL_DELETION_ORDER. #220

Closed Kircheneer closed 1 year ago

Kircheneer commented 1 year ago

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

Kircheneer commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

Kircheneer commented 1 year ago

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