generic diff view

[jedie]

django-reversion needs IMHO a easy way to add a diff view. We only have this few helper methods, but no prepared way to add them to the Admin, see: https://github.com/etianen/django-reversion/wiki/Generating-Diffs

I started a new branch to add a simple diff view to VersionAdmin(), here: https://github.com/jedie/django-reversion/compare/diff

I made some screenshots (from 03.05.2012): http://www.pylucid.org/de/about/screenshorts/django-reversion/

The idea is to only add a generic diff and a developer can simply overwrite the method VersionAdmin.make_diff() to make a more meaningful diff.

The current make_diff() implementation makes a difflib.ndiff() over a "pretty-print" representation of the model data. IMHO this is the only reasonable thing to do in a generic way, isn't it?

Maybe we can remove make_diff() from the VersionAdmin class and insert the links to the diff view only, if the inherited Admin class implement a make_diff() method?

[etianen]

I worry that this feature is going to be one of those where lots of people have different strong opinions about what the generic diff functionality should be. I also wonder if this isn't something of a minority feature, since your average website admin isn't really technical enough to understand what a diff is, or how to interpret it.

Additionally, it's going to be hard to diff a lot of custom fields. A WYSIWYG field, for example, would be almost impossible to generate a diff for. Or a m2m filter horizontal widget. Diffs only really make sense for large blocks of text.

[etianen]

Which is not to say that you're not doing a potentially very useful thing here. I just wonder whether this might not be best off in a separate django-reversion-admindiff project, rather than a fork of django-reversion itself. That way, you can build the best implementation of how you see this functionality, and people can use it if it suits them, without there being a canonical "reversion" way of doing it.

[jedie]

Yes, i know there can't be a generic diff view which makes sense to all type of models...

The most importand thing for me is to have a simple API for a diff view in django-reversion. The existing VersionAdmin.make_diff() is more a example.

What is your problem to merge the branch?

1.) You generally dislike a "diff API" and it must be moved to another external app?
2.) You dislike to enable the generic diff view as default?
3.) The generic diff view should be smater?

In case 2. i see a few solutions:
-We completely remove the default make_diff() method. It must be implemented from the django-reversion user.
-We move the default make_diff() into helpers and the django-reversion user can add it or a own method to his admin class.
-Everything stay as it is, we add a variable e.g.: "diff_view = False" to VersionAdmin for enable/disable the compare stuff in the history template (The view can be seens if someone construct the right URL)

btw. A idea for a better default make_diff():
We iterate over every field. Ignore all fields with the same values. Display field name as a html headline and display a separate ndiff of the value. (It would be cool to resolve related objects IDs to the instance representation, e.g. ForeignKey to user)

But again: Even if it looks better. A generic diff view can't be the best solution for every model. Nevertheless i would like to have a default diff ;)

[etianen]

I've got a few problems with it:

• Adds a dependency to an external diff library (this can be avoided with clever imports, of course).
• There are many "good" ways of doing a diff. You seem to be assuming that the entire model should be reduced to a single text diff. What about per-field diffs, for example? From my understanding, your API won't support this.
• Not enough people have asked for this to make me think it's a majority use case. Having it on by default seems wrong.
• Adding new views to a Django admin class is actually very easy. Generating a good diff is not. Providing support for adding the view, while leaving good diff generation mostly open to the user, seems wrong.
• I'd have thought the most elegant way to implement this would be with a subclass of VersionAdmin. This being the case, such a subclass can easily live in a separate app.

I think your work could exist very usefully as a drop-in subclass of VersionAdmin for people who require it. Having this as a core reversion feature won't make it much easier to integrate with user projects, but will add to the maintenance overhead of the project.

As a separate project, I'd of course link to it in the wiki as a reversion extension application. :)

[jedie]

I would not add another dependency: Use only external libs optional.

What do you exactly mean with "per-field diffs" ? Maybe the developer can make a diff only from one field in his own make_diff() method ?

As i sayed, IMHO it's impossible to generate a compare view for every model in a generic way... We can only give some example diff views and the developer can choose from those or make a own view.

I try to implement it as a drop-in subclass for VersionAdmin... IMHO the complete stuff is not "enough" for a seperate project.

[etianen]

An example of per-field diffs: A cms with a main content area and a sidebar, implemented as text fields. The diff view would ideally have two diffs, one for each content area.

I think you'll find that when you add unit tests, the associated CSS and JS, and a few compatibility shims with the most popular diff libraries, it'll make quite a tidy separate project.

[jedie]

I worked on the branch today and changes this:

1. renamed "diff" to "compare"
2. moved the "make compare" stuff into helpers
3. enhanced the compare result: Display field by field and create a diff

See: https://github.com/jedie/django-reversion/compare/diff

for 2.: I must moved the import of VersionAdmin, because it's build a import loop: https://github.com/jedie/django-reversion/blob/92465540a0b473326ba5cfaa81b0e34af7e91a5d/src/reversion/helpers.py#L21 How to fix this better?

for 3.: The diff looks more cleaner, compare:
OLD: http://www.pylucid.org/media/pylucid.org/screenshots_PyLucid/django-reversion/ReversionDiff_02.png
NEW: http://www.pylucid.org/media/pylucid.org/screenshots_PyLucid/django-reversion/ReversionDiff_04.png

OLD: http://www.pylucid.org/media/pylucid.org/screenshots_PyLucid/django-reversion/ReversionDiff_03.png
NEW: http://www.pylucid.org/media/pylucid.org/screenshots_PyLucid/django-reversion/ReversionDiff_05.png

TODO: How to use a better representation of the field values instead of using repr(), see: https://github.com/jedie/django-reversion/blob/92465540a0b473326ba5cfaa81b0e34af7e91a5d/src/reversion/helpers.py#L149

[jedie]

btw. cut out the compare stuff from VersionAdmin to a separate class is doable. But i don't prefer that. One Problem: How should the object_history.html looks like?

See two solutions:

1.) inherit from the original. But then i must add at least 4x a {% block ... %} area, for: Start form, table head, table body and end form.

2.) make a independent copy of object_history.html...

Both solutions are ugly, isn't it?

[etianen]

I don't see a problem with making a new copy of object_history. That's pretty much what I had to do when extending the original admin object_history template.

I'm wondering whether the thing to do here is to actually widen the scope of what you're trying to do. My ideal solution would be for this to be in a separate project, and for that project to take on ALL the diff responsibilities for reversion. The diff helpers in reversion would become deprecated and scheduled for removal. This would then make the django-reversion-diff project responsible for:

1. Low-level APIs for diff generation between two version objects.
2. An admin subclass that brings this diff generation to a handy web interface.

This would be a good solution for the current highly neglected diff helpers in reversion, and give more weight to the subproject.

Long-term, I don't want to have to understand and maintain more code in this project. Reversion has a tight scope that's entirely concerned with saving and retrieving versions. Offloading all diff responsibility would make it easier to maintain in the long run.

[jedie]

I didn't see, why is must be exist in a separate project.

This has several disadvantages: Users of django-reversion must install one project more and has to add one more app in the settings to INSTALLED_APP, TEMPLATE_DIRS... They must look in to separate documentation, has two projects for bug tracking etc.
And the both projects must track each other on code changes...

And that's for all small enhancement?

For me, it's an existential features that was on my list since a long time...

The only goal i see to create a separate project, is that you have less code to maintain. Correct my if i'm wrong...

[etianen]

GITHUB HAS GONE MAD!!!! :O

Anyway...

Surely it's just a case of adding another app to INSTALLED_APPS? The app template loader will then kick in and pull in the rest. Give the dependency on the external diff library for this code to work, it's not trivial to install in any case.

Maintaining the admin integration is currently a massive undertaking, since each new release of django breaks it anew. I don't want to take on any more complexity.

If it's written correctly, then all the diff code needs to do is subclass VersionAdmin, override history_view and add a few news URLs to the get_urls() method. The django admin framework was designed to make this sort of thing easy.

Keeping the documentation for Reversion small is fantastic for the users who only want the core feature set. Having an ecology of reversion extensions is great for people who need to take it further.

Django's been avoiding taking on any contrib apps for a while now. Some would consider a revert / rollback library a core feature for the django web framework, and a few years ago reversion was suggested for inclusion in Django core. Saner heads prevailed, and reversion remains a 3rd part extension. This leaves the community open to writing a better revert / rollback library, and reduces the development overhead for the Django core devs.

[jedie]

I really find no arguments for a separate project... No question: It's possible to make it as a second app. But what's the goal of this?

Between the lines i read that you didn't want to spend more time with django-reversion and hopes that some days there will come a better app ?

Did I misunderstand?

[etianen]

Absolutely not. Reversion is a core part of the libraries I use every day, and I spent around an hour a week maintaining it. Thankfully my current employer recognises the value of well-supported open-source technology and is happy for me to spend this time on it. I've deployed it on two new sites this week already.

I don't think your extension is sufficiently mainstream for inclusion in the core codebase. I have a responsibility to keep reversion running for as long as it's needed by the community, and part of that responsibility involves keeping the codebase lean.

If you're willing to create and maintain this as a reversion contrib app, then I'll happy help promote your efforts. Kindly stop insulting my own.

[jedie]

I'm glad to here that django-reversion has a feature... ;)

If you say you use it in own project, than one question: Have to a solution for a history compare? IMHO it's essential at least if more than one person can edit content... Say you want to create a simple Wiki. A compare view is essential for this, isn't it?

What's about this: In the core of django-reversion leaves only the "select to compare" stuff. But the view that's create the compare page must be implemented from the developer how used django-reversion in his project.

The example that only makes a ndiff over the object representation leaves as a example in the wiki...

[jedie]

OK, i have refactor the compare stuff, see: https://github.com/jedie/django-reversion/compare/diff#L0R472

Now, the admin class must inherit from CompareVersionAdmin instead of VersionAdmin to get the compare functionality.

Enhancement:

I added: compare_fields and compare_exclude to set a list of fields for compare or/and exclude fields, like: ModelAdmin.fields and ModelAdmin.exclude: https://docs.djangoproject.com/en/1.4/ref/contrib/admin/#django.contrib.admin.ModelAdmin.exclude

And now you can easy defined a own compare method for every field, simply by adding a new method, in this scheme: "compare_%s" % field_name.
If no method are defined it will used fallback_compare() (which makes the simple ndiff)

e.g.:

class MyModel(models.Model):
    date_created = models.DateTimeField(auto_now_add=True)
    user = models.ForeignKey(User)
    content = models.TextField()
    sub_text = models.ForeignKey(FooBar)

class MyModelAdmin(CompareVersionAdmin):
    def compare_date_created(self, obj, version1, version2, value1, value2):
        date1 = value1.isoformat()
        date2 = value2.isoformat()
        return "%s <-> %s" % (date1, date2)

I should insert examples for ForeignKey and other related fields.
Another idea: add a simple helper for WYSIWYG fields: Use striptags from django and existing ndiff...

[jedie]

Another update:

I added two ways to define own compare methods: The method name must be in this schemes:

1. "compare_%s" % field_name
2. "compare_%s" % field.get_internal_type()

So you can define generic compare for all the same type...

I have also refactor the helper.py and use google-diff-match-patch if installed. If not fallback with ndiff and a simple highlight without pygments.

[jedie]

i started a seperate project: https://github.com/jedie/django-reversion-compare

[etianen]

Nice... by forking like that, you'll keep the author history on the helpers code too.

When you get the project completed to your satisfaction, I'll take a look, then mark the diff helpers in reversion as deprecated.

[jedie]

I use https://github.com/jedie/django-reversion-compare in production, now.

There still needs some work to test every kind of model fields.
Seems that the google diff lib is the best solution for making diffs, but it's not perfect. On the other side: a better diff is a nice to have...

Take a look at it and think about to remove the helpers stuff in django-reversion and add a link to django-reversion-compare

[etianen]

I finally got around to checking out out your project!

So, I'd like to link to your project from the reversion docs. That's an easy decision.

I'm not so sure about removing the diff helpers methods. Now they're in, removing them would break backwards compatibility, and they're not doing any harm. Instead, it might be best if I just park them and direct any queries about improving the diff generation to your own project.

It would be beneficial if your own project had drop-in replacements for the three diff methods available in django-reversion. That way I could eventually deprecate them and point people your way.