django-addanother icon indicating copy to clipboard operation
django-addanother copied to clipboard

Fix #14

Open olivierdalang opened this issue 9 years ago • 9 comments

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 avatar Jun 14 '16 11:06 olivierdalang

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

One problem is the same than https://github.com/jonashaag/django-addanother/pull/12#issuecomment-225798192

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...

olivierdalang avatar Jun 14 '16 12:06 olivierdalang

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

jonashaag avatar Jun 14 '16 19:06 jonashaag

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

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

jpic avatar Jun 15 '16 07:06 jpic

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.

olivierdalang avatar Jun 15 '16 13:06 olivierdalang

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 ;)

jpic avatar Jun 15 '16 13:06 jpic

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 avatar Jun 15 '16 18:06 jonashaag

@olivierdalang Tests should work now.

jonashaag avatar Jun 20 '16 21:06 jonashaag

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

codecov-io avatar Oct 20 '16 06:10 codecov-io

/remind me to look into this in 14 days

jonashaag avatar Apr 10 '20 15:04 jonashaag