Skip to content

Fixed #18974 -- Deprecated models.permalink decorator #377

Closed
wants to merge 4 commits into from

3 participants

@dstufft
Django member
dstufft commented Sep 18, 2012

This Pull Request deprecates the models.permalink decorator as per #18974.

It also includes updates to the documentation to take note of the deprecation and to point users towards using the reverse method instead.

@dstufft dstufft Fixed #18974 -- Deprecated models.permalink decorator
Added a PendingDeprecationWarning when calling models.permalink.
Additionally refactored the documentation to replace suggestions
of using the permalink decorator with the reverse function and
include it in the Deprecation timeline.
c83dd96
@akaariai akaariai and 1 other commented on an outdated diff Sep 18, 2012
docs/ref/models/instances.txt
@@ -521,6 +525,8 @@ in ``get_absolute_url()`` and have all your other code call that one place.
The ``permalink`` decorator
~~~~~~~~~~~~~~~~~~~~~~~~~~~
+.. deprecated:: 1.5
+
The way we wrote ``get_absolute_url()`` above is a slightly violation of the
@akaariai
Django member
akaariai added a note Sep 18, 2012

This isn't true any more. The get_absolute_url() is non-DRY as it uses reverse()...

@dstufft
Django member
dstufft added a note Sep 18, 2012

Yea that's true. I didn't touch the permalink docs themselves other than adding the deprecated note. I wasn't sure if it would be better to leave them as is until it's removal, remove them completely, or just rewrite them to still document permalink but to mention that people should use reverse instead.

@dstufft
Django member
dstufft added a note Sep 18, 2012

To be more explicit, which way should the docs be changed?

@akaariai
Django member
akaariai added a note Sep 18, 2012

Well, that is a good question :) Maybe something along these lines:

deprecation reason: the permalink is deprecated, use get_absolute_url with reverse instead.

In early versions of Django there wasn't an easy way to use URLs defined in URLconf file inside get_absolute_url(). That meant you would need to define the URL both in URLConf and get_absolute_url(). Permalink was added to overcome this DRY principle violation. There is no reason to use permalink any more.

the actual docs for permalink here...

We should still have docs for the decorator until it is deprecated. The docs are a little verbose, but I don't see anything which must be reworded there...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@akaariai akaariai commented on the diff Sep 18, 2012
docs/releases/1.5.txt
@@ -377,3 +377,10 @@ The markup contrib module has been deprecated and will follow an accelerated
deprecation schedule. Direct use of python markup libraries or 3rd party tag
libraries is preferred to Django maintaining this functionality in the
framework.
+
+
+``django.db.models.permalink``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The :func:`~django.db.models.permalink` decorator has been deprecated. Use
@akaariai
Django member
akaariai added a note Sep 18, 2012

Do we need a more explicit guide on how to replace the permalink decorator? The answer might very well be no...

Maybe the permalink documentation should mention why it is deprecated. You can just use reverse inside the get_absolute_url, so you can always replace permalink by get_absolute_url which uses reverse.

@dstufft
Django member
dstufft added a note Sep 18, 2012

I don't think we need a more explicit guide. reverse() is documented and get_absolute_url() is documented, I think it should be fairly obvious what to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dstufft
Django member
dstufft commented Sep 18, 2012

Updated the changes to the documentation to remove the line calling out the implementation of get_absolute_url as violating DRY, since it no longer was, and to explicitly mention the reason for both the inclusion of permalink and the reason for it's removal.

@timgraham
Django member

Based on the discussion in the ticket, doesn't look like formal deprecation is going to happen, but I've pulled the relevant docs from this into a patch and attached it to the ticket for review. Thanks!

@timgraham timgraham closed this Nov 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.