Use URLObject for URL manipulation throughout. #112

Closed
tomchristie opened this Issue Jan 5, 2012 · 2 comments

Comments

Projects
None yet
2 participants
@tomchristie
Member

tomchristie commented Jan 5, 2012

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

This comment has been minimized.

Show comment
Hide comment
@j4mie

j4mie Jan 5, 2012

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.

Collaborator

j4mie commented Jan 5, 2012

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

This comment has been minimized.

Show comment
Hide comment
@tomchristie

tomchristie Jan 6, 2012

Member

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.

Member

tomchristie commented Jan 6, 2012

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.

@fjsj fjsj referenced this issue in fjsj/django-rest-framework Aug 12, 2016

Closed

test #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment