search

SmileyChris / easy-thumbnails

Easy thumbnails for Django
http://easy-thumbnails.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
1.39k stars 319 forks source link

Optimize without signals #537

Closed jrief closed 4 years ago

jrief commented 4 years ago

According to many docs about Django, the use of signals is only recommended if an app must handle events happening in another app. In Django Chat, Carlton Gibson for instance, advises against using signals to communicate events from the same app to itself.

This pull requests eliminates the the need for inter-app signal communication.

jrief commented 4 years ago

@Mogost Could you please peer review this PR.

Mogost commented 4 years ago

@jrief I need to take the time to make sense of this. I am concerned about the issue of backward compatibility, whether there will be problems with libraries like this.

jrief commented 4 years ago

@Mogost Thanks for the hint. Didn't know that library.

Mogost commented 4 years ago

@jrief Just tested on my project and this approach was failed. I have code like this (custom template tag):

@register.filter
def photo_thumbnail(user, size):
    try:
        photo_link = get_thumbnailer(user.photo)[str(size)].url
    except (EasyThumbnailsError, ValueError):
        photo_link = static('img/userpic.svg')
    return mark_safe(photo_link)

An exception occurred: RuntimeError: Model class easy_thumbnails.models.Source doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS.

IMHO, this change carries many different risks, all of which are rather difficult to foresee, at the same time I do not see very great benefit from these changes. UPD: Also should tests not be changed here? https://github.com/SmileyChris/easy-thumbnails/blob/master/easy_thumbnails/tests/settings.py#L24

jrief commented 4 years ago

Closing in favor of #559