jazzband / django-formtools

A set of high-level abstractions for Django forms
https://django-formtools.readthedocs.io
BSD 3-Clause "New" or "Revised" License
801 stars 135 forks source link

Remove usage of undocumented django.utils.function.lazy_property #84

Closed adamchainz closed 7 years ago

adamchainz commented 7 years ago

Whilst looking through Django core, I saw that the lazy_property function was unused and undocumented, so by Django's policies it can be removed. However I found that formtools relies on it from its last use in a commit, the removal of formtools: django/django@14a3b60981f63334520c713bb3a2c9c694c49a1f

I'm not sure of the correct way forward here but perhaps it should just be documented in Django core? Or should it be copied in here so we can tidy up core?

timgraham commented 7 years ago

It would be nice to see what the documentation looks like (it's difficult for me to understand the use case at a quick glance) and to check if the property is useful anywhere in Django. A quick GitHub search only revealed users who committed their virtual environment and the Django test suite.

adamchainz commented 7 years ago

The docstring for lazy_property pretty much explains it: https://github.com/django/django/blob/master/django/utils/functional.py#L415

I think it would be easy to remove here, replacing with just:

@property
def current_step(self):
    return self._get_current_step()

@current_step.setter
def current_step(self, value):
    return self._set_current_step(value)

Arguably this is clearer too as there isn't a new 'lazy property' concept when reading the code, it's just good ol' plain properties. Then it could be removed from django core.

timgraham commented 7 years ago

The only thought I have is that @property's getter, setter, and deleter attributes were added in Python 2.6 and this code was added when Python 2.5 was supported, so maybe this workaround was required at the time but isn't needed now. I'm fine with your plan if it works.

adamchainz commented 7 years ago

I think lazy_property was also designed to avoid a bit of boilerplate, but it's probably preferable to have that boilerplate twice in django-formtools than document it since this is the only place using it.