Fix bug in TypeConversionDict

[aaossa]

TypeConversionDict should raise KeyError during conversion when key is not present, but now is returning the default value. This PR fixes this bug and adds the corresponding test cases.

Fixes #1102

[ThiefMaster]

TypeConversionDict should raise KeyError during conversion when key is not present

No, it shouldn't. .get() is never supposed to raise a KeyError due to the key not existing.

The idea behind #1102 is that the conversion callback raising a KeyError does not result in the default value being used, i.e. catch a KeyError from rv = self[key] and then catch a ValueError from rv = type(rv)

[aaossa]

Mmm I see. So, what's the expected value of out in this code from the issue?

switch = {'a': 1}
data = TypeConversionDict(good='a', bad='b')
out = data.get('bad', 'default', lambda v: switch[v])
assert out != 'default'

(Reading the docstring) The requested data exists ('bad'), so the default value ('default') should not be used. type is provided and is a callable (lambda v: switch[v]), so it should try to convert the value (but won't be possible, so...), return the value ('b') or raise ValueError if conversion is not possible (in this case is not possible). What should be the value of out? When should ValueError be raised?

Thanks for your feedback @ThiefMaster , really fast 😁 👌

[untitaker]

Since your type raises a KeyError (or anything other than ValueError), it should bubble up.

ValueError coming from type is caught though.

[aaossa]

So something like:

try:
    rv = self[key]
except KeyError:
    rv = default
if type is not None:
    try:
        rv = type(rv)
    except ValueError:
        rv = default
return rv

should work?

[untitaker]

Right, though in line 4 I would return directly.

…

On Sat, Apr 15, 2017 at 09:05:25AM -0700, Antonio Ossa wrote: So something like: ```python try: rv = self[key] except KeyError: rv = default if type is not None: try: rv = type(rv) except ValueError: rv = default return rv ``` should work? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1106 (comment)

[aaossa]

Nice, I'll fix this in a few hours. Thanks for your comments!

[aaossa]

Already fixed it in my fork. Can you check if everything is ok now? If you aprove the changes, agains which branch should I make the PR?

[untitaker]

Yes this looks good. I think it would be better to do this in master. Please also add a changelog and a small testcase.

…

On Sat, Apr 15, 2017 at 10:01:46AM -0700, Antonio Ossa wrote: Already fixed it in [my fork](https://github.com/aaossa/werkzeug). Can you check if everything is ok now? If you aprove the changes, agains which branch should I make the PR? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1106 (comment)

[aaossa]

Added changes to changelog. The tests were already there in the previous commit 👌 Should use a new PR or re-open this one?

[untitaker]

It appears that I can't reopen this one, so please make a new one :)