Skip to content

Loading…

Use URLObject for URL manipulation throughout. #112

Closed
tomchristie opened this Issue · 2 comments

2 participants

@tomchristie
Owner

As of #111 we now use URLObject for URL Manipulation.

There's at least two other places we ought to now be using it:
https://github.com/tomchristie/django-rest-framework/blob/master/djangorestframework/views.py#L118
https://github.com/tomchristie/django-rest-framework/blob/master/djangorestframework/renderers.py#L315

Needs a quick look though the source to check if there's anywhere else that we should be doing the same.

May also want to use spurl (template tags around URLObject) to replace the add_query_param template tag.
(See also #108)

@j4mie
Collaborator

Re: https://github.com/tomchristie/django-rest-framework/blob/master/djangorestframework/views.py#L118

Two thoughts: (1) this is so simple, I'm not sure URLObject would gain you much here, to be honest. Maybe some unicode safety, but I can't see any immediate potential problems (2) I assume this nasty thread-level monkey-patching is needed to handle cases where reverse is being called somewhere where a request is not available? If so, perhaps some sort of post-processing on URLs to convert relative to absolute could be done? It feels like misusing set_script_prefix like this is bound to break something somewhere.. probably a discussion for another ticket..

Re: https://github.com/tomchristie/django-rest-framework/blob/master/djangorestframework/renderers.py#L315

Is there any test coverage for this? Randomly changing stuff in here doesn't seem to make any tests fail :( I'm guessing I should be writing tests before making the changes... any pointers?

Re: use of Spurl: can you see the requirement for this sort of URL manipulation inside templates increasing in the future? If not, introducing another dependency might be overkill. Maybe better to just replace the guts of add_query_param to use URLObject.

@tomchristie
Owner

Maybe better to just replace the guts of add_query_param to use URLObject.

Yup that'd be nicer.

I assume this nasty thread-level monkey-patching is needed to handle cases where reverse is being called somewhere where a request is not available?

It's there because originally I needed (or thought I needed) as ay to force reverse to return fully qualified URLs, so that for example, you could just hook into existing get_absolute_url methods.

It's grungy and needs to disappear, but just needs a bit of care to make sure that doing so isn't going to massively break existing code that's out there.

Is there any test coverage for this?

There's no tests for DocumentingTemplateRenderer at the moment. There should be.

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.