[WIP] Use Integral abc when checking for integer types
#294
Conversation
Allows for 3rd party libraries (like numpy) to register other integer types
|
The current practice of checking for specific types precludes the use of Before: In [2]: checker = Int64ValueChecker()
In [3]: checker.CheckValue(np.int64(1))
Traceback (most recent call last):
File "<ipython-input-3-c3f4d18106cd>", line 1, in <module>
checker.CheckValue(np.int64(1))
File "C:/temp/protobuf/python/google/protobuf/internal/type_checkers.py", line 123, in CheckValue
raise TypeError(message)
TypeError: 1 has type <type 'numpy.int64'>, but expected one of: (<type 'int'>, <type 'long'>)After: In [2]: checker = Int64ValueChecker()
In [3]: checker.CheckValue(np.int64(1))
Out[3]: 1L
In [4]: isinstance(np.int64(1), (int, long))
Out[4]: False
In [5]: checker.CheckValue(1.)
Traceback (most recent call last):
File "<ipython-input-5-8c7eaa978130>", line 1, in <module>
checker.CheckValue(1.)
File "C:/temp/protobuf/python/google/protobuf/internal/type_checkers.py", line 124, in CheckValue
raise TypeError(message)
TypeError: 1.0 has type <type 'float'>, but expected one of: (<type 'int'>, <type 'long'>) |
|
👍 cc @haberman |
|
Ping! Any chance this could make it into 3.0? |
dhirschfeld
changed the title
Integral abc when checking for integer typesIntegral abc when checking for integer types
|
This willl only work for the python backend - there's type checking going on in My attempt at a fix doesn't seem quite right as it results in template<class T>
bool CheckAndGetInteger(
PyObject* arg, T* value, PyObject* min, PyObject* max) {
PyObject* numbers = PyImport_ImportModule("numbers");
PyObject* Integral = PyObject_GetAttrString(numbers, "Integral");
bool is_integer = PyObject_IsInstance(arg, Integral) != 0;
if (!is_integer) {
FormatTypeError(arg, "integer");
return false;
}
#if PY_MAJOR_VERSION < 3
if (PyObject_Compare(min, arg) > 0 || PyObject_Compare(max, arg) < 0) {
#else
if (PyObject_RichCompareBool(min, arg, Py_LE) != 1 ||
PyObject_RichCompareBool(max, arg, Py_GE) != 1) {
#endif
PyObject *s = PyObject_Str(arg);
if (s) {
PyErr_Format(PyExc_ValueError,
"Value out of range: %s",
PyString_AsString(s));
Py_DECREF(s);
}
return false;
}
#if PY_MAJOR_VERSION < 3
if (!PyLong_Check(arg)) {
*value = static_cast<T>(PyInt_AsLong(arg));
} else // NOLINT
#endif
{
if (min == kPythonZero) {
*value = static_cast<T>(PyLong_AsUnsignedLongLong(arg));
} else {
*value = static_cast<T>(PyLong_AsLongLong(arg));
}
}
return true;
} |
|
Yikes, this got buried somehow, sorry for letting it sit so long! I agree with this change in principle. If it can be made to work with both the pure-Python and the Python-C++ implementations I'm happy to merge it. The change should have a test though (the test could define its own |
|
Sorry, missed the comment. I'm not working on this anymore so not sure if it's still relevant. Since this isn't a complete fix and I won't be looking at it any time soon I'll close this PR - I can always reopen if the issue is still present next time I come to use protobuf... |
Allows for 3rd party libraries (like numpy) to register other integer types