Skip to content

Pretty form #187

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

Merged
merged 12 commits into from
Apr 24, 2020
Merged

Pretty form #187

merged 12 commits into from
Apr 24, 2020

Conversation

MKaras93
Copy link
Contributor

Feature requested in issue #85

Added method pretty_form() to DeepDiff class. Method returns a string with verbose description of changes, for example:

Item root[5] added to dictionary.
Item root[3] removed from dictionary.
Type of root[2] changed from int to str and value changed from 2 to "b".
Value of root[4] changed from 4 to 5.

There is one text pattern for each available report_type.

@seperman
Copy link
Owner

@MKaras93 Awesome! Thanks.

@MKaras93
Copy link
Contributor Author

I've fixed the indent problem (caused by solving conflicts on GitHub), but locally tests stopped on TestIgnoreOrderDelta. When I comment this test out - it all passes, so maybe someone should look into it.

@MKaras93
Copy link
Contributor Author

Oh, I see it also fails on Python 3.5 because of f-strings. I will replace them with something else when I have some time.

@seperman
Copy link
Owner

Yep TestIgnoreOrderDelta is failing. Working on it right now. Thanks!

@seperman
Copy link
Owner

@MKaras93 TestIgnoreOrderDelta is passing now if you merge dev into your branch.

@MKaras93
Copy link
Contributor Author

@seperman Ready to merge.

@seperman seperman merged commit 8fe8c1c into seperman:dev Apr 24, 2020
@seperman
Copy link
Owner

@MKaras93
Thanks. Merged!

@seperman
Copy link
Owner

Hi @MKaras93
The more I think about it, the more it seems like the pretty should be a new view. We have the text view and tree view as you know. instead of running diff.pretty_form(), the user can either do view=pretty. What do you think? Is that cool with you?

@MKaras93
Copy link
Contributor Author

Hey @seperman
In issue #85 there was a discussion about this, and simonfontana argumented that views (TreeResult and TextResult) are dicts, while the purpose of this feature is a string representation:

Hi @seperman,
I'm interested in creating a PR if I get the time. I was looking at the code a bit and noticed that TreeResult and TextResult are dictionaries. I think a pretty format should be returned as string. Do you think it would it be better to create method instead, pretty(), which returns a string?
If we create a "pretty view" as you suggested and return it as string i think it would break consistency with current views and confuse users. If we return it as dict I don't think it would be pretty. I would like to hear your thoughts on this.

In the next comment you agreed to such implementation and that's why I implemented it this way.

Regardless of that, in my opinion methods are good for this purpose, but it's possible that I don't understand the idea of views in deepdiff correctly. For me views are just different representation of the same object - just like python standard __str__ and __repr__ methods. We have different outputs, but there is still tree underneath. I don't understand the point of requiring user to declare a single view beforehand, when instead we can provide simple interface to access all views.

Or am I missing something here? Happy to discuss - as I said, I don't 100% understand the concept of views in deepdiff :)

@seperman
Copy link
Owner

@MKaras93 Ah you are right. I did agree with that. Ok I will keep it a method. Views are currently different formats of representing the output and each one has its own features. The text view is the original output format but then some people were interested in having access to the tree of what objects were compared to what and traverse back to their ancestors etc. So it has way more capabilities than the text view. In my new PR, I'm adding a Delta view which is used for the new Delta module in DeepDiff.

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.

2 participants