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

routing: Transparently convert a MultiDict containing parameters. #724

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@houseofsuns
Copy link
Contributor

commented Apr 7, 2015

Sorry to nag once more w.r.t. the multiple values for URL building, but I think I found a way to make everybody happy. The following should allow passing in MultiDicts and receiving the desired behaviour without getting any regressions.
This should be ready to merge (as in there is test and documentation).

This allows to pass a MultiDict and get the intuitively expected result
(that is multiple parameters, where multiple values are present). This
allows to recreate a URL as follows:

  • create a URL with multiple values for a parameter
  • use Map.match() to extract them into a MultiDict
  • recreate the URL with Map.build() handing in the MultiDict

The last step would fail before this commit (in that it would reproduce only
one of the values for each parameter). Also this avoids the collateral
damage introduced by the last attempt at allowing MultiDicts.

This zaps one of the test cases in test_basic_building to reflect the
changed behaviour.

Markus Oehme
routing: Transparently convert a MultiDict containing parameters.
This allows to pass a MultiDict and get the intuitively expected result
(that is multiple parameters, where multiple values are present). This
allows to recreate a URL as follows:

* create a URL with multiple values for a parameter
* use Map.match() to extract them into a MultiDict
* recreate the URL with Map.build() handing in the MultiDict

The last step would fail before this commit (in that it would reproduce only
one of the values for each parameter). Also this avoids the collateral
damage introduced by the last attempt at allowing MultiDicts.

This zaps one of the test cases in test_basic_building to reflect the
changed behaviour.
@houseofsuns

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2015

Hm, I messed up the layout above ...

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 7, 2015

You can pass in multiple query params by providing a list of values as value. Isn't that good enough?

@houseofsuns

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2015

I have no strong feelings here, but I found the point that build() is then inverse to match() a big plus. For me this is more an enhancement/sugar coating so that everything works as I would intuitively expect it.

Also if not this change, I will have to modify some of my code to use lists of values, so I thought I might try here first, for the benefit of the general public. :)

veelai
Fix syntax for 2.6 compatability
Dict comprehension was added in 2.7, so we instead use a constructor.
@houseofsuns

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2015

I just saw the failure and fixed the syntax to be compatible to 2.6

valueiter = iteritems(values, multi=True)
temp = dict(
(k, (values.getlist(k)
if len(values.getlist(k)) > 1

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 10, 2015

Member

This is really slow, it calls getlist twice for no reason. I'll try to write some benchmarks, but the current code isn't acceptable (though I apprechiate it that there are no perf regressions for the non-multidict path)

This comment has been minimized.

Copy link
@houseofsuns

houseofsuns Apr 10, 2015

Author Contributor

What about the following? It is imho a tad uglier, but should be pretty optimal in performance (in that there is nothing done which could be spared and keep the functionality the same).

temp = {}
for key, list_value in values.lists():
    if len(list_value) == 1:
        temp[key] = list_value[0]
    else:
        temp[key] = list_value

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 11, 2015

Member

This looks good but please use iterlists from compat.

Markus Oehme
Improve performance of MultiDict conversion for routing
This uses a more explicit version than the more elegant comprehension to get
rid of the duplicate getlist call.
@houseofsuns

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Tentative bump. What about this, it looks like I'll actually not need this anymore, but I think it would be a sensible change anyway. :)

@untitaker untitaker added the ready label Mar 29, 2016

@edk0 edk0 referenced this pull request Apr 20, 2018

Merged

Optimize URL building #1281

@davidism

This comment has been minimized.

Copy link
Member

commented May 22, 2018

Issue history so I can keep this straight:

  • #658 was the initial request to support multiple values when building.
  • #660 converted the values to a MultiDict unconditionally.
  • #667 fixed an issue #660 caused when passing dicts.
  • #709 reported an issue with being able to indicate a single, iterable value vs multiple values.
  • #710 reverted #660 to return to the previous behavior.
@davidism

This comment has been minimized.

Copy link
Member

commented May 22, 2018

Continued in #1310

@davidism davidism closed this May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.