Skip to content

Conversation

@brandoncazander
Copy link

Fixes #2489.

The Django 1.4.x tox tests were failing as tests.urls was empty, so I included the necessary urlpatterns from test_relations.py.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if we put this in tests/versioning.py?
I'd also prefer if we can keep the urlpatterns isolated to the same module as the test.

@lovelydinosaur
Copy link
Contributor

Coming along nicely. One inline comment there.

@brandoncazander
Copy link
Author

@tomchristie I moved the test to the proper location and reset urls.py. I would appreciate your quick input on two things:

  1. Which inline comment are you referring to?
  2. You can see the build failing now. From my testing, in django.core.urlresolvers.RegexURLResolver urlconf_module is:
    • Django 1.4, 1.5: <module 'tests.urls' from 'django-rest-framework/tests/urls.pyc'>
    • Django 1.6, 1.7: <module 'tests.test_versioning' from 'django-rest-framework/tests/test_versioning.py'>

I'm not sure what's going on there. If you have time to take a look that would be great; otherwise, I'll continue on it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than returning a new match is it possible to just mutate the one we've gotten back?
Eg.

match = django_resolve(path, urlconf)
match.namespaces = match.namespaces[1:]
return match

@lovelydinosaur
Copy link
Contributor

Issue was due to using SimpleAPITestCase, presumably not using the full urlpatterns/resolvers machinery in that case.

I've gone with a slightly simpler implementation, based on your work, where we don't both to go for a full resolve tie in. Think it's a better trade off.

@lovelydinosaur lovelydinosaur merged commit 030f01a into encode:version-3.1 Feb 5, 2015
@lovelydinosaur
Copy link
Contributor

Thanks for all your work on this! :)

@brandoncazander
Copy link
Author

@tomchristie Thank-you! The resolve was a nice complement to reverse, but it was too much overhead for the one spot we need it right now. Sorry I wasn't available to finish off my work. :)

rpkilby pushed a commit to rpkilby/django-rest-framework that referenced this pull request Aug 7, 2017
lovelydinosaur pushed a commit that referenced this pull request Aug 7, 2017
* Add regression test for #2505. Thanks @pySilver!

* Add regression test for #5087

* Revert "Cached the field's root and context property."

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants