Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #14 #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

olivierdalang
Copy link
Contributor

This fixes #14

(I try to commit the unittest first, to see if it works properly with travis, the actual fix will come in a second commit)

@olivierdalang olivierdalang force-pushed the allow_custom_get_params branch from 1560e27 to 788af3a Compare June 14, 2016 12:02
@olivierdalang olivierdalang force-pushed the allow_custom_get_params branch from 788af3a to 4d6c813 Compare June 14, 2016 12:13
@olivierdalang
Copy link
Contributor Author

Oh nooooo, failing tests again :/ (under python 2.7 only it seems...)

One problem is the same than #12 (comment)

Another is probably because there's something bad in the testapp I committed :

RuntimeError: Conflicting 'team' models in application 'testapp': <class 'testapp.models.Team'> and <class 'test_project.testapp.models.Team'>.

I don't really get what's going on, and as I didn't manage to run tox locally, it's quite hard to work on this...

@jonashaag
Copy link
Owner

Thanks! Let's see if @jpic knows how to deal with this 😁 pytest is causing us a lot of trouble there.

I'll add some code review comments

@@ -5,15 +5,15 @@
{% if edit_related_url %}
<a class="related-widget-wrapper-link change-related" id="change_id_{{ name }}"
href=""
data-href-template="{{ edit_related_url }}?{{ url_params }}"
data-href-template="{{ edit_related_url }}{{ edit_params_separator }}{{ url_params }}"
Copy link
Owner

Choose a reason for hiding this comment

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

This feels a bit messy; how about we do all of the URL preparation stuff in Python and profit from urlparse? (Keep in mind to use six in that case)

@jpic
Copy link
Collaborator

jpic commented Jun 15, 2016

Moving test_project/tests.py to test_project/testapp/tests.py works.

For lecture, see https://code.djangoproject.com/ticket/22280

@olivierdalang olivierdalang force-pushed the allow_custom_get_params branch from f820e58 to 7d56cba Compare June 15, 2016 12:59
@olivierdalang
Copy link
Contributor Author

olivierdalang commented Jun 15, 2016

This feels a bit messy; how about we do all of the URL preparation stuff in Python and profit from urlparse? (Keep in mind to use six in that case)

I tried to improve it. Not sure what six would be good for there ?

And about the test issues/improvements, may I ask one of you to take care of it ? I'm struggling too much with it (I tried moving the file and adapting the tox.ini, but I still get tests.py not found)... I'll see how you deal with it and learn.

@jpic
Copy link
Collaborator

jpic commented Jun 15, 2016

Once you've moved test_project/tests.py into test_project/testapp/tests.py, also change py.test test_project/tests.py to py.test test_project/testapp/tests.py in tox.ini, otherwise tox should fail with "tests.py not found" message when running py.test from tox after moving the file ;)

@jonashaag
Copy link
Owner

jonashaag commented Jun 15, 2016

I tried to improve it. Not sure what six would be good for there ?

Still doing manual string fiddling. This is what I'm talking about, although I must admit it's much more involved than I initially expected it. Bad APIs in the Python standard library there!

from django.utils import http
from six.moves import urllib


def url_extend_query_string(url, params):
    """Extend the query string of `url` with the additional query string
    `params`.

    :param params: e.g. ``{"q": ["my search terms"]}``
    :type params: dict of str -> list(str)
    """
    a, b, c, d, qs, e = urllib.parse.urlparse(url)

    qs = urllib.parse.parse_qs(qs)
    for k, v in params.items():
        qs.setdefault(k, []).extend(v)
    qs = http.urlencode(qs, doseq=True)

    return urllib.parse.ParseResult(a, b, c, d, qs, e)

@jonashaag
Copy link
Owner

@olivierdalang Tests should work now.

@codecov-io
Copy link

Current coverage is 76.78% (diff: 100%)

Merging #15 into master will decrease coverage by 18.21%

@@             master        #15   diff @@
==========================================
  Files             4          4          
  Lines           100        112    +12   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits             95         86     -9   
- Misses            5         26    +21   
  Partials          0          0          

Powered by Codecov. Last update 2b7158e...7d56cba

@jonashaag
Copy link
Owner

/remind me to look into this in 14 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom get parameters
4 participants