Fix for iterable moved items what are found with iterable_compare_func#473
Conversation
To stay consistent with other types of reporting, moved items should be relative to t2. Also, moved items should branch deeper to look for more nested changes (similar to item added and removed).
|
I will work on getting unittests fixed soon which may require making some of my changes conditional on either a new flag or |
seperman
left a comment
There was a problem hiding this comment.
Hi @dtorres-sf
Thanks for making the PR.
I may be missing something. What is the benefit of going one level deeper if the item is "moved"? Is it because you want the flexibility when using iterable_compare_func ?
Usually when we report an object as "moved", it means nothing has changed about it except its path, so there is no more need to go deeper.
|
@seperman we still need to go deeper, here is an example a basic example: In the above example we have both moved top level items as well as changed the moved items. For With these changes here is the output:
|
…e_func. Fix unittests to represent the branching deeper
|
@seperman I fixed the unittest by gating one of my changes to only be used for iterable_compare_func. Also updating unittests to be correct. You can see more examples of why we need to branch deeper in the unittests updates: These unittests were originally written with the branch deeper in mind - this shows that iterable_compare_func used to go deeper and show the nested values being changed: |
|
@dtorres-sf Thanks for fixing the tests. Do you think we should add a parameter that allows the user to decide whether we should go deeper when using iterable_compare_func or not? |
|
@seperman We could, I am trying to think of the use cases of when they would not want to go deeper. If they don't go deeper they will see that items moved, but not the changes that happened inside the moved items. This effectively misses some of the differences deepdiff is used to find. Do you remember why that The changeset that made the change mentions using difflib but not sure why stopping the branch deeper was added as part of that. |
|
@seperman Just wanted to follow up here to see if this is a change that you think we could move forward with? If you think it is best to add a parameter for these changes I am fine with that and can get that added. |
|
Hi @dtorres-sf |
|
@dtorres-sf I think that was added to make it consistent with the rest of the flow where moving means the object has not changed. |
seperman
left a comment
There was a problem hiding this comment.
I'm going to merge it and add the continue part below when we are not using iterable compare func.
| # If the item was moved using an iterable_compare_func then we want to make sure that the index | ||
| # is relative to t2. | ||
| reference_param1 = j | ||
| reference_param2 = i |
There was a problem hiding this comment.
I would make it continue if not self.iterable_compare_func
|
@dtorres-sf DeepDiff 8.0.0 is published and it includes your contribution. Thank you! |
|
@seperman Thank you so much!! I will try it out and let you know if there are any issues |
Our usage of deepdiff heavily involves iterable_compare_func and we have been unable to upgrade past 6.1.0 because of an issue that was introduced in this changeset.
There are two key changes:
This PR should fix this issue:
#414