Support quoting list values in mappings

[Lonami]

What do these changes do?

These changes resolve aio-libs/aiohttp#4714.

Are there changes in behavior for the user?

Yes, users can now use lists or tuples when quoting mappings.

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> (e.g. 588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the PR
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

[Lonami]

I haven't checked performance but if it's an issue it can be improved by re-implementing _query_var inside the new _query_seq_pairs. And if performance is an absolute concern, we may use the trick where we avoid global look-ups to things like isinstance or the tuple/list types.

---

Edit: running python bench.py before (9afd40d):

Cython quote ascii: 5.317 sec
Python quote ascii: 5.436 sec
Cython quote PCT: 6.746 sec
Python quote PCT: 6.564 sec
Cython quote: 21.134 sec
Python quote: 21.215 sec
Cython unquote: 2.949 sec
Python unquote: 2.933 sec

Running now:

Cython quote ascii: 5.139 sec
Python quote ascii: 5.130 sec
Cython quote PCT: 6.492 sec
Python quote PCT: 6.499 sec
Cython quote: 21.151 sec
Python quote: 21.101 sec
Cython unquote: 2.845 sec
Python unquote: 2.859 sec

Probably within noise, it's not like I changed any code the bench touches anyway.

[Lonami]

With regards to:

Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

Is that necessary? Taking a quick look at the commit log, it seems to be used inconsistently anyway (and many commits don't follow that convention).

If it's desired I can rebase and force push, otherwise maybe I'll send another PR updating it.

[Lonami]

Oh, I hadn't seen the CHANGES/.TEMPLATE.rst file because it starts with a dot so it was hidden. Was I supposed to use that in the changes? If yes, it could be a good idea to rename it to something else and add comments indicating "WRITE HERE" or something.

[webknjaz]

Oh, I hadn't seen the CHANGES/.TEMPLATE.rst file because it starts with a dot so it was hidden. Was I supposed to use that in the changes?

No, that template is used by towncrier to generate CHANGES.rst (which we run before making releases). You should usually add a short change note file and that's it. Here's an example https://github.com/aio-libs/yarl/blob/01e529690fbb6d122e4a4a6575e1aa5edba4ce47/CHANGES/389.feature, check how it looks in https://github.com/aio-libs/yarl/blob/master/CHANGES.rst#features.

[webknjaz]

Is that necessary? Taking a quick look at the commit log, it seems to be used inconsistently anyway (and many commits don't follow that convention).

It's not about the commit messages but about the changes to docs.

[Lonami]

I've also added test for edge cases where the list/tuple are empty or have a single item which we didn't have before. I think it should be ready for review again.

[Lonami]

Bumping this now that we're on the weekend. Is there any reason not to merge this other than lack of time?

[webknjaz]

@Lonami I'd like tests and mentions in all docs for a[]=1&a[]=2

[Lonami]

Yeah don't mind the extra space there it's fixed in the next commit which I'll push once I polish.

[Lonami]

mentions in all docs for a[]=1&a[]=2

Changing the test to compare str() actually did reveal that this escapes [] in the key when we shouldn't, although this is a bug from before. That is, this test fails in master:

def test_stuff():
    url = URL("http://example.com/path")
    u2 = url.with_query([("a[]", "3"), ("a[]", "4")])

    assert str(u2) == "http://example.com/path?a[]=3&a[]=4"

With:

E       AssertionError: assert 'http://examp...D=3&a%5B%5D=4' == 'http://examp...h?a[]=3&a[]=4'
E         - http://example.com/path?a[]=3&a[]=4
E         ?                          ^^    ^^
E         + http://example.com/path?a%5B%5D=3&a%5B%5D=4
E         ?                          ^^^^^^    ^^^^^^

tests/test_update_query.py:228: AssertionError

So I suppose it's fine to fix this because people relying on it being escaped was buggy (not intended) behaviour. I'm not sure if this should be fixed here or as a separate patch.

[Lonami]

Opened #445 to address that.

[webknjaz]

As per #445 (review), use encoded values for the square brackets in tests. It is still useful to have tests with non-letter values.

[Lonami]

I have "ported" the test using a list of tuples from https://github.com/aio-libs/yarl/pull/445/files#diff-ae226bc0f15cb48ba935d31884c1034dR226 over the new parametrize from here, tested also a single key, and fixed the test above to use the quoted braces.

[Lonami]

Hooray 🎉, thanks!