Opened 9 years ago

Closed 8 years ago

#2606 closed enhancement (fixed)

[patch] Tag for URL reverse lookup

Reported by: Ivan Sagalaev <Maniac@…> Owned by: adrian
Component: Template system Version:
Severity: normal Keywords: reverselookups
Cc: Maniac@…, gary.wilson@…, rushman@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

A template tag for doing reverse URL lookup. Supports only positional arguments for now. Patch follows.

Attachments (6)

2606.diff (2.4 KB) - added by Ivan Sagalaev <Maniac@…> 9 years ago.
Patch
2606.2.diff (2.8 KB) - added by Ivan Sagalaev <Maniac@…> 9 years ago.
Support for kwargs
2606.3.diff (3.0 KB) - added by Ivan Sagalaev <Maniac@…> 8 years ago.
Support for referring views outside project
2606.4.diff (6.2 KB) - added by Ivan Sagalaev <Maniac@…> 8 years ago.
Patch + docs + tests
views.py (159 bytes) - added by Ivan Sagalaev <Maniac@…> 8 years ago.
tests/regressiontests/templates/views.py
urls.py (290 bytes) - added by Ivan Sagalaev <Maniac@…> 8 years ago.
tests/regressiontests/templates/urls.py

Download all attachments as: .zip

Change History (20)

Changed 9 years ago by Ivan Sagalaev <Maniac@…>

Patch

comment:1 Changed 9 years ago by Ivan Sagalaev <Maniac@…>

  • Cc Maniac@… added
  • Summary changed from Tag for URL reverse lookup to [patch] Tag for URL reverse lookup

Changed 9 years ago by Ivan Sagalaev <Maniac@…>

Support for kwargs

comment:2 Changed 9 years ago by Ivan Sagalaev <Maniac@…>

Added support for keyword arguments in {% url %}, docstring updated.

However I have a hard time figuring out how to write tests for this thing. The tag uses current urlconf from settings to do its job and I'm not sure if it would be correct to replace it with special testing urls.py and views.py... So for now there are no tests for {% url %} in the patch.

comment:3 Changed 8 years ago by jeroen_at_lbvd.nl

Very usefull tag. I would propose a small change though. In the render function you prepend the project_name to the viewname (return reverse(project_name + '.' + self.view_name, args=args, kwargs=kwargs)).

However, this assumes that the application is part of the project (in the python sense). If the app is in a python package that is NOT the current project this aproach fails. This could be the case if you 'borrow' an application from another project (without copying it).

For example, I have an 'library project' (magicstrings) that contains various apps, used by different projects. The actual projects are little more than configuration and some very project-specific code. Thus the python path to a view in one of the apps part of Magic Strings is in my case (regardless of the project they are actually used in) magicstrings.app_name.views.view_name. And this patch puts, wrongly, the name of the actual project in front of that.

Another example is contrib. If you try to reverse map django.contrib.admin.views.main.index, this approach would try to reverse map current_project.django.contrib.admin.views.main.index, which would obviously fail.

There are imo two solutions: a) let people always specify the projectname or b) perhaps it is an idea to change the tag so that you can add the projectname in front of the path, separated by e.g. a colon. Option a) is the simplest, but perhaps I miss the reason behind the current behavior and is b) the only option.

{% url my_project:app_name.views.view_name %}

Would result in reverse('my_project.app_name.views.view_name'), and if projectname is ommited, fallback to the current one:

{% url app_name.views.view_name %}

Would result in reverse('current_project.app_name.views.view_name')

Rgds,
Jeroen

comment:4 Changed 8 years ago by Ivan Sagalaev <Maniac@…>

First of all thank you for a very useful comment!

You wrote: "perhaps I miss the reason behind the current behavior".

The only reason is that I indeed wrongly had in mind only project's local views.

Looking at your proposals I can think of a third way that can be both short for in-project views and suitable for out-of-project views. It'll be like this:

  1. try to import specified path without modifications
  2. if that fails, add the project name and try again

I'll make an updated patch shortly.

Changed 8 years ago by Ivan Sagalaev <Maniac@…>

Support for referring views outside project

comment:5 Changed 8 years ago by jeroen_at_lbvd.nl

Good solution. Thanks!

comment:6 Changed 8 years ago by jacob

This is very nice. Can you please add unit tests and documentation so we can check this in?

comment:7 Changed 8 years ago by Ivan Sagalaev <Maniac@…>

I have a problem making a unit test for it. Every other template tag can be tested on its own but {% url %} actually needs urls.py and views.py... I can't think of a good way to fake this environment.

As for the docs, I'll post the patch shortly. Thanks!

comment:8 Changed 8 years ago by Ivan Sagalaev <Maniac@…>

I've wrapped my head around testing and made a new patch (2606.4.diff) with tests and documentation.

The patch requires 2 new files (attached). I tried to include them in the patch itself but couldn't apply it then, so I decided to attach them as is.

Changed 8 years ago by Ivan Sagalaev <Maniac@…>

Patch + docs + tests

Changed 8 years ago by Ivan Sagalaev <Maniac@…>

tests/regressiontests/templates/views.py

Changed 8 years ago by Ivan Sagalaev <Maniac@…>

tests/regressiontests/templates/urls.py

comment:9 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Cc gary.wilson@… added

comment:10 Changed 8 years ago by Simon G. <dev@…>

  • Keywords reverselookups added
  • Needs documentation set
  • Triage Stage changed from Unreviewed to Accepted

I think this is just awaiting some brief documentation, but looks good to go.

comment:11 Changed 8 years ago by Ivan Sagalaev <Maniac@…>

It does have docs right in the patch :-). (the last one -- 2606.4.diff)

And to get you on course: two attached files views.py and urls.py are for tests. I didn't include them in the patch because I beleive they wouldn't be created by the 'patch' tool itself. But may be I'm mistaken :-)

comment:12 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Accepted to Ready for checkin

Ahh. Didn't scroll down that far. Now that I know what this does, I like it very much.

comment:13 Changed 8 years ago by Sergey Kirillov <rushman@…>

  • Cc rushman@… added

comment:14 Changed 8 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [4494]) Fixed #2606 -- Added tag for working out the URL of a particular view function.
All work done by Ivan Sagalaev.

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