Add PyErr::take method

[messense]

Just like Option::take.

If no error is set, the existing PyError::fetchmethod returns a SystemError makes it inconvenient to use in Rust side for example when setting the cause for PyErr: https://github.com/messense/rjsonnet-py/blob/e144e9c08621135e03d06c8d0fc53af51ff51b3c/src/lib.rs#L224-L233

[davidhewitt]

Hmmm. I agree this is useful, but I feel that PyErr::fetch() -> Option<PyErr> is probably the more accurate representation of the underlying PyErr_Fetch API.

At the moment the SystemError comes from the "normalizing" process internally. We could remove that hidden "feature" and make some kind of function like this to use inside the PyO3 codebase:

pub(crate) fn api_call_failed(py: Python) -> PyErr {
    PyErr::fetch(py).unwrap_or_else(|| PySystemError::new_err("exception value missing"))
}

Though TBH maybe it's a serious enough issue if the error is missing that we should panic.

[messense]

Hmmm. I agree this is useful, but I feel that PyErr::fetch() -> Option<PyErr> is probably the more accurate representation of the underlying PyErr_Fetch API.

Yes, It feels weird that PyErr::fetch() doesn't return a Option<PyErr>, although changing it is a breaking change.

[davidhewitt]

Agreed it would be breaking. I'm not sure how many downstream users would be affected that, I suspect most don't use PyErr::fetch directly?

Perhaps let's put this on the list to decide for 0.15 - either we can add this new take(), or we can consider breaking fetch()? If doing so can lead to simpler code I think that's worth experimenting with.

(For now it doesn't look too hard to implement pyerr_take() as local helper function, even if it's mildly annoying to repeat in multiple crates...)

[messense]

I don't mind waiting for 0.15, will experiment with PyErr:fetch() -> Option<PyErr>.

[davidhewitt]

@messense should we close this now that it looks like #1717 will be merged for 0.15?

[messense]

Sure.