refactor parse_url_overrides

[russellballestrini]

Refactor parse_url_overrides:

• pop values with default if key is missing
• change conditionals to test for truth
• prevent throwing an exception if passing keyword with default value
• test if anchor starts with '#' before blindly adding it

[digitalresistor]

Looks good :D

[digitalresistor]

Please fix this PEP8 issue:

https://travis-ci.org/Pylons/pyramid/jobs/229568331#L176

[russellballestrini]

Hi @mmerickel and @bertjwregeer - what should I do next?

[mmerickel]

Sorry @russellballestrini, thanks for the reminder. This logic needs to be uniform between resource_url and route_url. Currently resource_url does this on its own but ideally it would be updated to use parse_url_overrides as well by munging the argument names or something.

Also annoyingly .pop is slower than if foo in bar: bar.get(foo) but I think it's not worth fretting about performance here.

[tseaver]

Also annoyingly .pop is slower than if foo in bar: bar.get(foo) but I think it's not worth fretting about performance here.

dict.pop(key, default) is a mutating operation: it isn't equivalent to get(key, default), which should itself be faster than the if key in thing: foo = thing[key] variant.

[russellballestrini]

The original version uses dict.pop(key) and intentionally mutates the kwargs. This actually surprised me and so I was considering using dict.get(key, default) when talking about it in IRC. Do we know why this original version uses pop? Could we (or should we) change it to get?

[tseaver]

@russellballestrini The point of dict.pop is to consume the item, so that it is no longer present in the dict during later processing. This usage is particularly important when proxying to another function / method using **dict.

It is perfectly OK to use dict.pop(key, default) to avoid the if statement (similar to dict.get(key, default)).

[russellballestrini]

@tseaver - Ah yes, I see how popping and mutating the kwargs could be useful, especially if we have multiple functions working on the same data with separate concerns.

@mmerickel - I see what you mean about resource_url having the same embedded logic - I'll take a stab at refactoring that too!

[mmerickel]

Sorry I meant pop in my example... the issue is that the pop function call is significantly slower than the if foo in bar test just because of python being python and that annoys me because the hot route is a falsey test here but it's fine. Ignore I said anything.

[russellballestrini]

@mmerickel - the resource_url function is for traversal and thats hard for me to reason about. I'm still up for the challenge but it will take me a bit longer.

[mmerickel]

@russellballestrini it's the same code it's just the parameters have different names (no leading underscores).

[mmerickel]

I went ahead and did the work to cleanup and standardize this effort with resource_url. I only disagree with one part of this change and as such I reverted it. I do like the addition of the test to only prefix the anchor with # if the anchor doesn't already start with #. While # isn't technically allowed in an anchor, I do not like the precedent this would set and the inconsistency it would allow with other parts of the API. Guessing what the user wants here feels odd and there were no added docs to back it up.

[mmerickel]

Found a bug while adding a test... yup. _anchor=None would cause a type error still. Fixed now.

I accidentally pushed this PR as a branch on pyramid which is why the CI tests are messing up but the /pr ones are the ones we care about and they're passing.

[russellballestrini]

Very cool - I always love a change with more deletes then adds and testing code is most of the adds!

[digitalresistor]

👍