Return object refs #43
Conversation
…s. Non-subscriptable ones are broken after this commit, but there's no test for it. So let's pretend everything's fine.
…efore_return_object_refs
… they are subscriptable
…passing type_change, value_change and item_added_and_removed tests
…, but still not the repeat repetition ones
…rt_repetition, test_list_of_unhashable_difference_ignore_order*, test_list_of_sets_difference_ignore_order*, test_list_that_contains_dictionary, test_comprehensive_ignore_order, test_ignore_order_when_objects_similar
…ore_order_report_repetition
…t_custom_object_with_added_removed_methods, test_custom_objects_add_and_remove_method, test_custom_objects_add_and_remove_method_verbose, test_set_of_custom_objects, test_dictionary_of_custom_objects
…t. We're checking twice now - once before starting comparison and once before reporting. This handles descending deeper into the comparison. Optimizable.
…t-style report from the ref-style one.
…result(). Now also passing test_bad_attribute.
4 similar comments
|
@victorhahncastell Wow, this is some major PR! Looking into it. Thanks. |
| self._from_ref_repetition_change(ref) | ||
|
|
||
| def _from_ref_default(self, ref, type): | ||
| if type in ref: |
There was a problem hiding this comment.
It is generally recommended not to use builtin names as variable names. Maybe type should be called something else like ref_item or something?
|
I really like the idea of having |
| for hash_value in hashes_removed: | ||
| for i in t1_hashtable[hash_value].indexes: | ||
| change_level = level.branch_deeper(t1_hashtable[hash_value].item, None, | ||
| child_relationship_class=SubscriptableIterableRelationship, # TODO: that might be a lie! |
|
Thanks for reviewing! I'm glad you're happy with the basic design. I fixed the shadowed keyword and gonna look into handling non-subscriptable iterables next. What's your view on how to present them in the text-style view? You suggested inserting a star to indicate this over in PR #42 . On the other hand, we might also just accept that text-style output is a bit fuzzy and optimized to be readable rather than strictly correct or parseable. |
|
@victorhahncastell I made a dev branch so we can work on this together. I changed the PR to the dev branch. |
…cess param but this is okay as it's the ChildRelationship subclass' responsibility to handle this correctly.
|
Great! I fixed the handling of non-subscriptable iterables to produce backwards compatible text output. Still need to check up on my own code and bring test coverage back up. Feel free to help me review if you've got time :) Do you see additional issues we need to address at this point? |
|
Hey @victorhahncastell , |
|
@seperman Sure, it's an honor :) |
|
@victorhahncastell Awesome! I added you as a collaborator. It should show up that you have an invitation to the DeepDiff repo. :) |
Hi Sep,
I finally got around to implement the feature I talked about earlier.
I changed DeepDiff to provide and use internally a new, less processed view of a DeepDiff result. This "reference-style" view provides a change as a linked list of the object path leading to the actual change, allowing access to all objects in this path on both sides of the comparison.
An individual result is now of the new type "DiffLevel", which is the leaf of this linked list. I tried to depict the broad idea in refdesign.odt (which is kinda ugly and should probably be removed before merging this).
The basic idea of this was to allow a program employing DeepDiff to further process it's results. For example, I'm currently using DeepDiff in a script that fetches and displays an overview of permission data in some web application. I use DeepDiff to figure out where this data has changed. Afterwards, I mark the data DeepDiff has found as added or deleted in an HTML view.
Used in this way DeepDiff can free the developer from writing custom comparison code.
The new reference-style view for DeepDiff allows me to do something like this:
permdata = self.model.permissions olddata = self.cmp.permissions diff = DeepDiff(olddata, permdata, default_view='ref') # enable new view if 'set_item_removed' in diff: for change in diff['set_item_removed']: parentset = change.up.t2 parentset.add("<del>" + change.t1 + "</del>")With my changes DeepDiff now uses this new structure to recreate the traditional result format. All existing tests pass and I'm positive this is fully compatible to the way DeepDiff behaved previously.
I know this is a huge change to DeepDiff's internals but I hope you can see the benefit of it :)
There are still a bunch of open TODOs in the code but I'd love to work with you to get this merged.
Best,
Victor