linuxsoftware / ls.joyous

A calendar application for Wagtail
BSD 3-Clause "New" or "Revised" License
74 stars 35 forks source link

Joyous EventBase.status field clashes with Django-Model-Utils StatusModel.status field #35

Open linuxsoftware opened 4 years ago

linuxsoftware commented 4 years ago

@sam-mi reports:

Event.status fieldname clashes with Django Model Utils Model.status field making subclasses of ls.joyous EventBase incompatible with any project that already implements Model Utils StatusField in their models.

I assume https://github.com/jazzband/django-model-utils is the correct homepage for Django Model Utils?

Pull request https://github.com/linuxsoftware/ls.joyous/pull/34

This would break backwards compatibility for Joyous so would have to go in a 2.0 release.

linuxsoftware commented 4 years ago

I have never used django-model-utils, but would it not be easier just to use a StatusField with a different name instead of using the StatusModel mixin? e.g.

class DeliveryPage(joyous.models.EventBase):
    delivery_status = StatusField(_('status'))
    delivery_status_changed = MonitorField(_('status changed'), monitor='delivery_status')

etc?

linuxsoftware commented 4 years ago

Closing this issue as I have had no response

sam-mi commented 4 years ago

Sorry, I got pulled away.

No, it would not be easier because that breaks interoperability with other libraries that already rely on ModelUtils via model mixins etc. Django Model Utils is an established library that has set a precedent in the Django community that status on a model is generally a CharField that stores values the the DB rather than a property method that calculates a value.

Besides it's much easier to modify the name of a calculated property than to change the name of a model field that is inherited from an installed library, especially once migrations have been run.

I think this simple change is valuable as it improves Joyous compatibility with an existing and well established library and as such improves interoperability with other apps as well, which can only help the uptake of LS.Joyous - which is an excellent library btw!

sam-mi commented 4 years ago

I have another branch that I could submit a PR from if you think it would be useful?

It's a much larger change - refactoring models as abstract models that can be extended in the final project - no field changes, no migrations, updated tests, custom templates etc, actually quite possibly all non-breaking changes. I have done this to allow me to integrate registration and ticket sales on event pages within my wagtail implementation - which was not possible using the current version of LS.Joyous, at least not while extending my other page types as well.

linuxsoftware commented 4 years ago

Where I would like to get to is where there was just one Event type and one Exception type. There is a discussion document on this in the wiki. Code changes towards that goal would be welcome. This will be a major change though, so will need to wait until 2.0.