bpo-39728: enum with invalid value results in ValueError twice #18641
Conversation
| Color('error out') | ||
| except ZeroDivisionError as exc: | ||
| self.assertTrue(isinstance(exc.__context__, ValueError)) |
There was a problem hiding this comment.
Hm, I hadn't noticed but this test would (still) be affected by the fix. And it seems reasonable to keep : \
Instead of changing when context is added to the exception, could the default implementation of _missing be changed to return None? Or would that break something?
There was a problem hiding this comment.
I think that would break logic because None might be as value of enum:
from enum import Enum
class Color(Enum):
BLUE = 'blue'
RED = 'red'
UNDEFINED = NoneThere was a problem hiding this comment.
But in that example _missing_ would need to return cls.UNDEFINED instead of None, because it has to return an instance of cls.
There was a problem hiding this comment.
Maybe you're right) I will try it
There was a problem hiding this comment.
The original verification here should be kept. It's checking that if using _missing_ explodes when searching for a value, the final result is the ValueError (of the bad value) and the ZeroDivisionError (of the internal problem).
de44c06 to
72eb2c4
Compare
| @@ -1776,7 +1776,8 @@ def _missing_(cls, item): | |||
| # trigger not found | |||
| return None | |||
| self.assertIs(Color('three'), Color.blue) | |||
| self.assertRaises(ValueError, Color, 7) | |||
| with self.assertRaises(ValueError): | |||
There was a problem hiding this comment.
This statement passes on the old code as well. Is it possible to specifically assert that a ValueError has been raised, but without the second ValueError being raised in the process?
There was a problem hiding this comment.
Yes, that can (and should) be tested, with something like this:
with self.assertRaises(ValueError) as cm:
Color(7)
self.assertIsNone(cm.exception.__context__)There was a problem hiding this comment.
Thanks for this work!
Probably the fix is not good, as one test is changed but it shouldn't. In the process, other test can be improved, see conversation.
All that said, note that one test is missing, which would be the one that verifies that in the simple case of value not being found, the exception is simple and not chained (which is what describes the original bug, which was not using the _missing_ feature).
Thanks again!
| @@ -1776,7 +1776,8 @@ def _missing_(cls, item): | |||
| # trigger not found | |||
| return None | |||
| self.assertIs(Color('three'), Color.blue) | |||
| self.assertRaises(ValueError, Color, 7) | |||
| with self.assertRaises(ValueError): | |||
There was a problem hiding this comment.
Yes, that can (and should) be tested, with something like this:
with self.assertRaises(ValueError) as cm:
Color(7)
self.assertIsNone(cm.exception.__context__)| Color('error out') | ||
| except ZeroDivisionError as exc: | ||
| self.assertTrue(isinstance(exc.__context__, ValueError)) |
There was a problem hiding this comment.
The original verification here should be kept. It's checking that if using _missing_ explodes when searching for a value, the final result is the ValueError (of the bad value) and the ZeroDivisionError (of the internal problem).
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
dorosch, thank you for your efforts. The source of the problem was actually in the default |
https://bugs.python.org/issue39728