Fix BaseFloat validation to match Float validation #1595
Conversation
This comment has been minimized.
This comment has been minimized.
| """ | ||
| if type(value) is float: # fast path for common case | ||
| return value | ||
| try: |
There was a problem hiding this comment.
This is where the bug is: this code misses the chance to convert objects that have an __index__ method.
| @@ -3339,7 +3339,7 @@ validate_trait_integer( | |||
| */ | |||
|
|
|||
| static PyObject * | |||
| as_float(PyObject *value) | |||
| validate_float(PyObject *value) | |||
There was a problem hiding this comment.
IIUC, the internals of validate_float e.g. PyFloat_AsDouble and others handle __index__ correctly, which is why this is the right approach - which is also what you meant by
difficulty of expressing the condition to be accepted as a float in pure Python
Is that correct?
There was a problem hiding this comment.
Yes! Moreover, the Python behaviour changes from version to version, and we want to match Python's behaviour regardless of Python version. The easiest way to do that is to use Python directly rather than trying to encode Python's logic in our own code (which would then mean having to update that code whenever Python's behaviour changes).
Related: https://bugs.python.org/issue40801
|
Still LGTM |
* Do duck-typed Complex validation * Fix skip condition * Rename for consistency with #1595
The validations of the
FloatandBaseFloattrait types previously didn't match: in particular, for Python >= 3.8,Floataccepts objects whose type is equipped an__index__method, whileBaseFloatdoes not. That was partly to do with the difficulty of expressing the condition to be accepted as a float in pure Python.This PR sidesteps that difficulty by exposing the
Floatvalidation check to Python via a new private functiontraits.ctrait._number_to_float.See also #1594, which makes essentially the same change to ensure that
ComplexandBaseComplexmatch. These two PRs will cause merge conflicts with each other, but they shouldn't be too difficult to resolve.Checklist