Opened 12 years ago
Last modified 11 months ago
#20757 new New feature
A more Object-Oriented URLResolver
Reported by: | Flavio Curella | Owned by: | |
---|---|---|---|
Component: | Core (URLs) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | flavio.curella@…, Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Pull Requests: | How to create a pull request | ||
Description ¶
Currently, the RegexURLResolver
is populated using tuples.
Because of that, code that tries to inspect the resolver will end up containg awkward indexing like this (semplified code for clarity's sake):
def get_urlformat(urlname): """ Given a URL name, returns the URL as a string suitable for string.format. Example:: urlpatterns = patterns('', url(r'^extra/(?P<extra>\w+)/$', empty_view, name="named-url2"), ) >>> get_urlformat('named-url2') 'extra/%(extra)s/' """ resolver = get_resolver() return resolver.reverse_dict[urlname][0][0][0]
My proposal is to replace tuples with a tuple-like object whose elements can be accessed by using attribute names. That way, the above method could become:
def get_urlformat(urlname): """ Given a URL name, returns the URL as a string suitable for string.format. Example:: urlpatterns = patterns('', url(r'^extra/(?P<extra>\w+)/$', empty_view, name="named-url2"), ) >>> get_urlformat('named-url2') 'extra/%(extra)s/' """ resolver = get_resolver() urlbit = resolver.reverse_dict[urlname].urlbits[0] return urlbit.format
I realize this is mostly aesthetic, and there could be performance implications since the URLResolver is probably the most hit codepath, so I appreciate every kind of opinion.
The attached patch is still a draft, it definitely needs more extensive test coverage, but it's a start to get the idea.
According to the ticket's flags, the next step(s) to move this issue forward are:
- To provide a patch by sending a pull request. Claim the ticket when you start working so that someone else doesn't duplicate effort. Before sending a pull request, review your work against the patch review checklist. Check the "Has patch" flag on the ticket after sending a pull request and include a link to the pull request in the ticket comment when making that update. The usual format is:
[https://github.com/django/django/pull/#### PR]
.
Change History (14)
by , 12 years ago
Attachment: | patch_commit_44e7837a1706.patch added |
---|
comment:1 by , 12 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Core (URLs) |
Type: | Uncategorized → New feature |
comment:2 by , 12 years ago
Needs tests: | set |
---|
comment:3 by , 12 years ago
Patch needs improvement: | set |
---|
by , 12 years ago
Attachment: | patch_commit_8a8468f33b16.patch added |
---|
by , 12 years ago
Attachment: | patch_commit_86a42e712a0e.patch added |
---|
comment:4 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Hi,
Python already has a namedtuple
class in its collections
module which is what you should be using for this case [1].
In theory, a namedtuple is a drop-in replacement for a regular tuple so backwards-compatibility should be assured.
However, I'm worried about the performance cost of such a replacement: as you noted, RegexURLResolver
is a pretty critical component of django.
From a quick search, there's nothing in our documentation that explains how to extend the URL resolver (or why you'd want to).
I've also never done it myself either so I don't see exactly what we'd gain with this change.
So, to sum up, I'm -0 on the idea of using namedtuples, provided there's no major performance hit.
On the other hand, I'd definitely be +1 on documenting the process of extending the URL resolver and its machinery.
[1] http://docs.python.org/2/library/collections.html?highlight=collections.namedtuple#collections.namedtuple
comment:5 by , 11 years ago
I've updated the patch to use namedtuples and rebased against 7523e784633b7757fbc82df58f80b197eeed988a.
Re performance: according to the Python docs 'Named tuple instances do not have per-instance dictionaries, so they are lightweight and require no more memory than regular tuples.'.
I'd still like to run some profiling to have a better idea of the impact of this change. Memory consumption might be the same, but instantiation could have an overhead.
comment:6 by , 11 years ago
I've done a quick benchmark. The code and raw results can be found here: https://gist.github.com/ulope/a63aff42f51b08181f72#file-results-txt
Here is a table of the results (n=1000000):
Python | operation | datastructure | time |
---|---|---|---|
CPy 2.6 | instantiate | tuple | 0.243867874146 |
instantiate | namedtuple | 0.791953086853 | |
access | tuple | 0.342396020889 | |
access | namedtuple | 1.09635186195 | |
CPy 2.7 | instantiate | tuple | 0.300357103348 |
instantiate | namedtuple | 0.817892074585 | |
access | tuple | 0.397746801376 | |
access | namedtuple | 1.12735199928 | |
CPy 3.3 | instantiate | tuple | 0.19853064499329776 |
instantiate | namedtuple | 0.8839332649949938 | |
access | tuple | 0.2663196460052859 | |
access | namedtuple | 1.1236915799963754 | |
CPy 3.4 | instantiate | tuple | 0.2100249359937152 |
instantiate | namedtuple | 0.707535210007336 | |
access | tuple | 0.2940493019996211 | |
access | namedtuple | 0.9200455860118382 | |
PyPy 2.3 | instantiate | tuple | 0.00525689125061 |
instantiate | namedtuple | 0.00653004646301 | |
access | tuple | 0.00449419021606 | |
access | namedtuple | 0.00840997695923 |
From this it seems that namedtuple indeed is noticeably more expensive than a plain tuple (except, unsurprisingly, on PyPy). However as the absolute times for namedtuple are still quite short even at one million iterations I think the impact on Django's URL resolvers should be negligible.
comment:7 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I'll address this as part of https://groups.google.com/forum/#!topic/django-developers/WQoJN_MGTOg.
comment:8 by , 10 years ago
Has patch: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:9 by , 11 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:10 by , 11 months ago
Cc: | added |
---|
handle IndexError correctly