Code

Opened 5 years ago

Closed 5 years ago

#9965 closed (wontfix)

Change import in permalink decorator from function to module

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

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 jcassee 5 years ago.
Patch to change import of reverse function to import of urlresolvers module

Download all attachments as: .zip

Change History (5)

Changed 5 years ago by jcassee

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

comment:1 follow-up: Changed 5 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to mtredinnick
  • Patch needs improvement unset
  • Status changed from new to assigned

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 Changed 5 years ago by jcassee

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.

comment:3 in reply to: ↑ 1 Changed 5 years ago by jcassee

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 Changed 5 years ago by jacob

  • Resolution set to wontfix
  • Status changed from assigned to closed

Yeah, supporting monkeypatching isn't a priority.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.