Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PyResult::ok_or_is_instance_of method #4819

Open
LilyFoote opened this issue Dec 23, 2024 · 1 comment
Open

Add PyResult::ok_or_is_instance_of method #4819

LilyFoote opened this issue Dec 23, 2024 · 1 comment

Comments

@LilyFoote
Copy link
Contributor

A situation I repeatedly find myself in is wanting to handle one specific error type and raise all others. For example:

fn current_app(py: Python, request: &Option<Py<PyAny>>) -> PyResult<Py<PyAny>> {
    let none = py.None();
    let request = match request {
        None => return Ok(none),
        Some(request) => request,
    };
    match request.getattr(py, "current_app") {
        Ok(current_app) => return Ok(current_app),
        Err(e) if e.is_instance_of::<PyAttributeError>(py) => {}
        Err(e) => return Err(e),
    };
    let resolver_match = match request.getattr(py, "resolver_match") {
        Ok(resolver_match) => resolver_match,
        Err(e) if e.is_instance_of::<PyAttributeError>(py) => return Ok(none),
        Err(e) => return Err(e),
    };
    resolver_match.getattr(py, "namespace")
}

In this case, I think it could be more readable as:

fn current_app(py: Python, request: &Option<Py<PyAny>>) -> PyResult<Py<PyAny>> {
    let none = py.None();
    let request = match request {
        None => return Ok(none),
        Some(request) => request,
    };
    if let Ok(current_app) = request.getattr(py, "current_app").ok_or_isinstance_of::<PyAttributeError>(py)? {
         return Ok(current_app);
    }
    let resolver_match = match request.getattr(py, "resolver_match").ok_or_isinstance_of::<PyAttributeError>(py)? {
        Ok(resolver_match) => resolver_match,
        Err(_) => return Ok(none),
    };
    resolver_match.getattr(py, "namespace")
}

The main advantage of this, in my opinion, is that is avoids the boilerplate handling of Err(e) => return Err(e). When multiple such error handling blocks are adjacent, this adds up to a lot of unnecessary noise.

In my project, I can implement this as:

use pyo3::prelude::*;
use pyo3::type_object::PyTypeInfo;

trait PyResultMethods<T> {
    fn ok_or_isinstance_of<E>(self, py: Python<'_>) -> PyResult<PyResult<T>>
    where
        E: PyTypeInfo;
}

impl<T> PyResultMethods<T> for PyResult<T> {
    fn ok_or_isinstance_of<E>(self, py: Python<'_>) -> PyResult<PyResult<T>>
    where
        E: PyTypeInfo,
    {
        match self {
            Ok(obj) => Ok(Ok(obj)),
            Err(e) if e.is_instance_of::<E>(py) => Ok(Err(e)),
            Err(e) => Err(e)
        }
    }
}

but it might be useful enough to other PyO3 users to upstream.

@davidhewitt
Copy link
Member

Interesting idea. I think what we're looking for is something like except AttributeError as e equivalent from Python? I'm not entirely sure what fits best, it took several readings for me to figure out the control flow.

For this particular case with getattr, there happens to be PyObject_GetOptionalAttr added in Python 3.13 which should (I think) be more efficient anyway for attribute lookup which you expect to fail.

Even if we move forward with a pyresult extension, adding the efficient path as an alternative to getattr for 3.13+ seems helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants