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

Looking to get your input on some refactoring #42

Conversation

victorhahncastell
Copy link
Contributor

Hi Sep,

this is not really a true PR in the sense that I'm not really looking to get it merged on it's own merit. I did perform some refactoring in preparation for additional changes and I'm looking to get your thoughts on this.

The picture is that I'm working on a version of DeepDiff that is able to return object references instead of text (or rather in addition to -- those are really separate features). DeepDiff is an incredibly powerful tool and this will open the door to use it in productive code. This will really help with any scenario where DeepDiff's result is to be processed in some way and not just displayed to the user.

To prepare for this I tried to streamline the result handling process a bit.

I also noticed that DeepDiff and DeepSearch (really cool idea BTW, already helped me a bunch) don't share much code at the moment. What are your thoughts on deriving them from a common base class?

Best,
Victor

@coveralls
Copy link

coveralls commented Aug 19, 2016

Coverage Status

Coverage decreased (-2.9%) to 97.126% when pulling 24bc125 on victorhahncastell:refactor_before_return_object_refs into 4af3c69 on seperman:master.

'oldvalue': 'old_value'}


def order_unordered(data):
Copy link
Owner

Choose a reason for hiding this comment

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

This is not used anymore since we switched to ContentHash instead of Pickle.

@seperman
Copy link
Owner

seperman commented Aug 19, 2016

Hey Victor,

Thanks for contributing to DeepDiff and your comments!

  • I really like your idea about DeepDiff and DeepSearch (and DeepHash) to share the same base class.
  • I'm not sure how to return object references in Python since it doesn't have pointers. I think currently we return the actual object in addition to its path if you set the verbose_level=2. What reference do you have in mind?
  • As you pointed, it might not be the best to return index number for generators. But I'm not sure what to use instead of that. Maybe putting a star will help? so for example in your test, {'iterable_item_removed': {'root[2]': 31337}} should become {'iterable_item_removed': {'root[*2]': 31337}}. If there are several items that have removed for example, it is gonna be: {'iterable_item_removed': {'root[*2]': 31337, 'root[*3]': 'something'}}. What do you think?

Let me know your thoughts.
Cheers,
Sep

@victorhahncastell
Copy link
Contributor Author

I'm gonna close here as we've also got #43 open now and I think I ended up not really using much of my "pre-refactoring" anyway... :)

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.

3 participants