[RFC] split PyErr::new() into multiple methods #4413
Conversation
|
Related: I have been wondering for a while about a new call syntax and you have prompted me to actually write that up: #4414 Maybe we could have I think |
|
Good point... I'm not fond of the names anyway, especially the Could |
birkenfeld
force-pushed
the
pyerr_constructors
branch
3 times, most recently
from
92b2dc7
to
b61862f
Compare
|
Changed the names to
|
Introduces new_empty(), new_arg() and new_args(), and the same on Exception types as new_err_empty(), new_err_arg() and new_err_args(). After the deprecation cycle, new_arg() could be renamed back to new(), since it is the one used in the vast majority of cases. I kept `PyErrArguments`, but it's now only implemented for tuples, not for all Python-convertible types.
birkenfeld
force-pushed
the
pyerr_constructors
branch
from
b61862f
to
01c1f29
Compare
There was a problem hiding this comment.
Thanks very much for pushing this forward, and I'm sorry I've been slow to engage deeply with the design.
I've added some comments with some reflections and ideas. I also opened #4584 after this PR prompted some additional thoughts.
| PyErr::from_state(PyErrState::Lazy(Box::new(move |py| { | ||
| PyErrStateLazyFnOutput { | ||
| ptype: T::type_object(py).into(), | ||
| pvalue: PyTuple::new(py, &[arg.into_py(py)]).into(), |
There was a problem hiding this comment.
It seems unfortunate that we have to (understandably) wrap everything inside a PyTuple to make things behave as intended here.
I wonder if there's performance justification to have a special case for e.g. strings (I assume one-arg strings are 99% of the calls).
There was a problem hiding this comment.
I mean, sure, we can do a check and only wrap if it's a tuple...
| /// Creates a new [`PyErr`] of this type with no arguments. | ||
| /// | ||
| /// [`PyErr`]: https://docs.rs/pyo3/latest/pyo3/struct.PyErr.html "PyErr in pyo3" | ||
| #[inline] | ||
| #[allow(dead_code)] | ||
| pub fn new_err_empty() -> $crate::PyErr { | ||
| $crate::PyErr::new_empty::<$name>() | ||
| } |
There was a problem hiding this comment.
It always felt weird to me that the function was e.g. PyValueError::new_err() -> PyErr and there is no obvious way to immediately create a ValueError.
I wonder, should we have e.g. PyValueError::new() -> Bound<'py, PyValueError> which can then be cast .into() PyErr? TBH, that might not be great either, having the .into() calls would feel quite clumsy.
Is there use for a macro here? e.g. return Err(py_err!(PyValueError(1))) or return py_err!(PyValueError(1, 2, 3))? That way we could automatically select the right constructor function according to the number of arguments.
All food for thought 🤔
There was a problem hiding this comment.
Could the into() directly give a Result<T, PyErr>? That might be ergonomic enough for many use cases.
As for the macro, I'm not sure it provides enough convenience to justify itself. Maybe yes if it reused the more general framework of a call macro?
Intended to start a discussion, about APIs, but also names :)
Similar to call(), introduces new0(), new1() and new_args(), and the same on Exception types as new_err0(), new_err1() and new_err_args().
After the deprecation cycle, new1() could be renamed back to new(), since it is the one used in the vast majority of cases.
I kept
PyErrArguments, but it's now only implemented for tuples, not for all Python-convertible types.