Opened 16 years ago

Closed 16 years ago

#9965 closed (wontfix)

Change import in permalink decorator from function to module

Reported by: Joost Cassee Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: dev
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The permalink decorator uses the urlresolvers.reverse function to generate a permanent link to objects. It imports the reverse function itself instead of the urlresolvers module. This causes trouble in projects that also use the localeurl application, because this application monkey-patches the reverse function in order to add a prefix. Although monkey-patching a core Django function is bad form, it is currently the only way I see to provide this functionality. Additionally, importing elements from a module instead of the module itself is slightly frowned upon in general (Python docs, Python FAQ: "Guido van Rossum recommends...", Fredrik Lundh). It is common in Django, though, so that might not impress anyone.

The attached patch changes the permalink decorator to import the urlresolvers module instead of the reverse function. Because the change is small, I hope the patch will be accepted.

Attachments (1)

9965-r9701.diff (596 bytes ) - added by Joost Cassee 16 years ago.
Patch to change import of reverse function to import of urlresolvers module

Download all attachments as: .zip

Change History (5)

by Joost Cassee, 16 years ago

Attachment: 9965-r9701.diff added

Patch to change import of reverse function to import of urlresolvers module

comment:1 by Malcolm Tredinnick, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick
Status: newassigned

Urgh ... ugly. :-(

The patch is okay, but it's not at all justified by allowing this sort of change. We should be aiming to fix the real problem, not wallpaper over and around it. In short, we need a better solution for dynamic reversing. I've been working on this from a few different angles lately, so I think we can do better here.

comment:2 by Joost Cassee, 16 years ago

Ugly indeed! I actually tried to come up with a way to integrate with the URL resolvers in a different way. My API thoughts were along the lines of http://groups.google.com/group/django-multilingual/msg/16ef5785c82af0d7. I will gladly wait for your changes.

in reply to:  1 comment:3 by Joost Cassee, 16 years ago

Replying to mtredinnick:

This issue is no longer a practical concern, as I found out (via Artiom Diomin) that doing the patch in models.py changes the importing order and fixes the problem. You can close this ticket, although I was hoping it would keep me updated on your dynamic reversing work. :-)

comment:4 by Jacob, 16 years ago

Resolution: wontfix
Status: assignedclosed

Yeah, supporting monkeypatching isn't a priority.

Note: See TracTickets for help on using tickets.
Back to Top