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

Add test for unicode keys and values #46

Conversation

doismellburning
Copy link

This implements the test from the comments of
#32 and demonstrates a regression
(bug was reported in 0.3.mumble, fixed in 0.3.8, and is now broken in
0.4.1 and 0.4.2)

This implements the test from the comments of
gruns#32 and demonstrates a regression
(bug was reported in 0.3.mumble, fixed in 0.3.8, and is now broken in
0.4.1 and 0.4.2)
@doismellburning
Copy link
Author

Looks like it got removed in 73ea2da (Py3 support)

@doismellburning
Copy link
Author

(Re 9150ffc, is there a reason why the tests seem to only use assert? I'm conscious I'm deviating from internal style there, but it was that or hand-duplicate the logic...)

@doismellburning
Copy link
Author

Also, re query string parameters, according to https://github.com/gruns/furl/blob/c1cc3e16e00fd9007a96ddeca6689595a5215d9b/API.md#query

all interaction with params should take place with decoded strings.

but

furl/tests/test_furl.py

Lines 498 to 525 in c1cc3e1

def setUp(self):
# All interaction with parameters is unquoted unless that
# interaction is through an already encoded query string. In the
# case of an already encoded query string like 'a=a%20a&b=b',
# its keys and values will be unquoted.
self.itemlists = list(map(itemlist, [
[], [(1, 1)], [(1, 1), (2, 2)], [
(1, 1), (1, 11), (2, 2), (3, 3)], [('', '')],
[('a', 1), ('b', 2), ('a', 3)], [
('a', 1), ('b', 'b'), ('a', 0.23)],
[(0.1, -0.9), (-0.1231, 12312.3123)], [
(None, None), (None, 'pumps')],
[('', ''), ('', '')], [('', 'a'), ('', 'b'),
('b', ''), ('b', 'b')], [('<', '>')],
[('=', '><^%'), ('><^%', '=')], [
("/?:@-._~!$'()*+,", "/?:@-._~!$'()*+,=")],
[('+', '-')], [('a%20a', 'a%20a')], [('/^`<>[]"', '/^`<>[]"=')],
[("/?:@-._~!$'()*+,", "/?:@-._~!$'()*+,=")],
]))
self.itemdicts = list(map(itemdict, [
{}, {1: 1, 2: 2}, {'1': '1', '2': '2',
'3': '3'}, {None: None}, {5.4: 4.5},
{'': ''}, {'': 'a', 'b': ''}, {
'pue': 'pue', 'a': 'a&a'}, {'=': '====='},
{'pue': 'pue', 'a': 'a%26a'}, {'%': '`', '`': '%'}, {'+': '-'},
{"/?:@-._~!$'()*+,": "/?:@-._~!$'()*+,="}, {
'%25': '%25', '%60': '%60'},
]))
appears to include int keys/values

Which is "correct"? (This turns out to impact my fix)

@gruns
Copy link
Owner

gruns commented Jan 15, 2015

With respect to assert vs self.assertEqual(): their behavior is identical -- they both raise AssertionError -- but the former is easier to read and maintain (no parentheses to match). Please use assert over self.assertEqual().

@gruns
Copy link
Owner

gruns commented Jan 15, 2015

With respect to int (and other non-string) keys and values as query string parameters: non-string query keys and values are coerced to strings with str(). This makes sense: query strings are strings -- only strings can be contained therein.

That intention of all interaction with params should take place with decoded strings is to instruct the user not to use encoded strings. It should mention that non-strings will be coerced to strings somewhere in the documentation, though. That is confusing. I'll ruminate on where that should be stated.

@gruns
Copy link
Owner

gruns commented Jan 16, 2015

I'll fix this regression shortly.

gruns pushed a commit that referenced this pull request Jan 16, 2015
@gruns
Copy link
Owner

gruns commented Jan 16, 2015

This regression is fixed in the latest furl v0.4.3.

>>> from furl import furl
>>> f = furl('http://site4dads.ru/')
>>> f.args[u'testö'] = u'testä'
>>> f.url
'http://site4dads.ru/?test%C3%B6=test%C3%A4'

Upgrade with

pip install furl --upgrade

I added Unicode tests similar to yours to make sure this regression doesn't occur again. Thank you for bringing this to my attention Kristian.

@gruns gruns closed this Jan 16, 2015
@doismellburning
Copy link
Author

Thanks

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.

None yet

2 participants