Implement `eyre` feature #1893

mejrs · 2021-09-29T23:59:23Z

davidhewitt

Thanks, looks great! Should we merge this at the same time as #1822 ?

davidhewitt · 2021-10-01T07:51:05Z

Cargo.toml

 # must stay at 0.3.x for Rust 1.41 compatibility
+eyre = {version = ">= 0.4" , optional = true}


Think these need to be swapped (new dep has split comment from its target). Also, should we place an upper bound?

Suggested change

# must stay at 0.3.x for Rust 1.41 compatibility

eyre = {version = ">= 0.4" , optional = true}

eyre = {version = ">= 0.4, <0.5" , optional = true}

# must stay at 0.3.x for Rust 1.41 compatibility

(oops)

Eyre is at 0.6.5 now, so that upper bound is too strict. 0.4 is the first version that has the eyre::Report type (?), so I think any version above that should be fine.

I'll admit that I don't know anything about how cargo resolves dependency versions, but this looks like how you should say "works with any eyre version of 0.4 and up". Is that too liberal and should it be pinned to ">= 0.4, <0.7" instead?

I think that by default cargo will pick the highest eyre possible for the pyo3 dependency, not the one which the user already has in their own Cargo.toml. (This is a bit awkward.) The user can adjust it manually using some incantation of cargo update --precise (like we do in our MSRV CI jobs).

Is that too liberal and should it be pinned to ">= 0.4, <0.7" instead?

Generally we've only offered a range for dependencies when the MSRV support requires an older version. I'd be tempted to ask the other question - is just supporting eyre = "0.6" acceptable?

As for the upper bound: if we were to take semver literally, eyre 0.7 could hypothetically be very breaking and completely remove the eyre::Report type. (Although I would be suprised if this happened in practice.) So I'd definitely prefer have an upper bound of < 0.7 if we go for a range. Dependabot can remind us to update when 0.7 releases 😄

davidhewitt · 2021-10-01T07:57:49Z

src/conversions/eyre.rs

+//! // A wrapper around a Rust function.
+//! #[pyfunction]
+//! fn py_open(filename: PathBuf) -> PyResult<Vec<u8>> {
+//!     // The `?` ("try") operator performs the conversion
+//!     // into a `PyErr` - if it is an error.
+//!     let data = rust_open(filename)?;
+//!     Ok(data)
+//! }


I think it should be possible to also just have a #[pyfunction] which returns eyre::Result<Vec<u8>> - which might be useful to have as an example (as a kind of test)?

I thought about doing that but I want to point at the where the conversion happens. A #[pyfunction] returning eyre::Result<Vec<u8>> has that conversion hidden somewhere in the pyfunction macro.

That's a fair point. I wonder if your second example "using eyre in general" could be tweaked slightly so that the function decompress is a pyfunction returning eyre::Result? The conversion from PyErr -> eyre::Result can still be there.

Alternatively maybe it's better to keep away from that here, and instead in the guide's function.md there is probably scope for an improved "error handling" section which comments on how to use Result with error types that convert to PyErr. (And can use Result<_, eyre::Report> as an example?)

Actually, I do want a pyfunction returning eyre's error. I think it is too easy for readers to assume that a pyfunction must return a PyResult and they have to wrap functions. I'm not sure if that was your reasoning though.

I do want to expand the error handling in the guide, but not with this PR.

davidhewitt · 2021-10-01T07:59:11Z

Oh, also this is definitely worth a CHANGELOG line :)

davidhewitt · 2021-10-04T07:11:04Z

CHANGELOG.md

 - Add `abi3-py310` feature. [#1889](https://github.com/PyO3/pyo3/pull/1889)

+


Bonus newline!

Suggested change

davidhewitt · 2021-10-04T07:12:55Z

CHANGELOG.md

@@ -22,8 +22,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add `PyMapping` type to represent the Python mapping protocol. [#1844](https://github.com/PyO3/pyo3/pull/1844)
 - Add commonly-used sequence methods to `PyList` and `PyTuple`. [#1849](https://github.com/PyO3/pyo3/pull/1849)
 - Add `as_sequence` methods to `PyList` and `PyTuple`. [#1860](https://github.com/PyO3/pyo3/pull/1860)
+- Add `eyre` feature to convert `eyre::report` into `PyErr`. [#1893](https://github.com/PyO3/pyo3/pull/1893)


Suggested change

- Add `eyre` feature to convert `eyre::report` into `PyErr`. [#1893](https://github.com/PyO3/pyo3/pull/1893)

- Add `eyre` feature to convert `eyre::Report` into `PyErr`. [#1893](https://github.com/PyO3/pyo3/pull/1893)

Also for dependencies I quite like to have something in the Packaging section along the lines of "added optional eyre dependency" - what do you think of having that as well (or instead)?

davidhewitt · 2021-10-04T07:20:48Z

Cargo.toml

 # must stay at 0.3.x for Rust 1.41 compatibility
+eyre = {version = ">= 0.4" , optional = true}


I think that by default cargo will pick the highest eyre possible for the pyo3 dependency, not the one which the user already has in their own Cargo.toml. (This is a bit awkward.) The user can adjust it manually using some incantation of cargo update --precise (like we do in our MSRV CI jobs).

Is that too liberal and should it be pinned to ">= 0.4, <0.7" instead?

Generally we've only offered a range for dependencies when the MSRV support requires an older version. I'd be tempted to ask the other question - is just supporting eyre = "0.6" acceptable?

As for the upper bound: if we were to take semver literally, eyre 0.7 could hypothetically be very breaking and completely remove the eyre::Report type. (Although I would be suprised if this happened in practice.) So I'd definitely prefer have an upper bound of < 0.7 if we go for a range. Dependabot can remind us to update when 0.7 releases 😄

davidhewitt

Thanks, all looks really good. Sorry for the delay - I was ill over the weekend. Feeling a bit more capable this morning finally!

Just commented on the remaining points with some thoughts. All is great as-is though so also happy if you want to call this done for now and merge it!

davidhewitt · 2021-10-04T07:24:20Z

src/conversions/eyre.rs

+//!     let text = decompress(bytes)?;
+//!
+//!     println!("The text is \"{}\"", text);
+//! # assert_eq!(text, "You should probably use the libflate crate instead.");


davidhewitt · 2021-10-04T07:27:52Z

src/conversions/eyre.rs

+//! // A wrapper around a Rust function.
+//! #[pyfunction]
+//! fn py_open(filename: PathBuf) -> PyResult<Vec<u8>> {
+//!     // The `?` ("try") operator performs the conversion
+//!     // into a `PyErr` - if it is an error.
+//!     let data = rust_open(filename)?;
+//!     Ok(data)
+//! }


That's a fair point. I wonder if your second example "using eyre in general" could be tweaked slightly so that the function decompress is a pyfunction returning eyre::Result? The conversion from PyErr -> eyre::Result can still be there.

Alternatively maybe it's better to keep away from that here, and instead in the guide's function.md there is probably scope for an improved "error handling" section which comments on how to use Result with error types that convert to PyErr. (And can use Result<_, eyre::Report> as an example?)

davidhewitt

Perfect, thanks! Feel free to merge this at your will (at the same time as finishing up #1822?)

CHANGELOG.md

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

mejrs · 2021-10-13T22:12:00Z

at the same time as finishing up #1822?

I don't see a reason to let this sit around here. I think we'd still want to have them both in 0.15 though.

mejrs added 2 commits September 30, 2021 01:35

Implement eyre feature

ad64767

Punctuation

96175d4

davidhewitt approved these changes Oct 1, 2021

View reviewed changes

mejrs and others added 6 commits October 1, 2021 15:28

Add eyre entry in changelog

59b5a4c

Add eyre feature entry to guide

2967220

Merge branch 'main' into eyre

56ee92f

Set eyre upper bound and move comment back

5685a96

Merge branch 'eyre' of https://github.com/mejrs/pyo3 into eyre

fff764c

Add eyre feature to docs.rs metadata

61c0041

davidhewitt reviewed Oct 4, 2021

View reviewed changes

mejrs marked this pull request as draft October 8, 2021 00:42

mejrs and others added 2 commits October 13, 2021 14:15

Merge branch 'PyO3:main' into eyre

fb5e264

Finish up review suggestions

ff69449

mejrs marked this pull request as ready for review October 13, 2021 12:26

mejrs requested a review from davidhewitt October 13, 2021 12:26

davidhewitt approved these changes Oct 13, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

d999a67

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

davidhewitt enabled auto-merge (squash) October 13, 2021 22:19

davidhewitt merged commit ae87369 into PyO3:main Oct 13, 2021

davidhewitt mentioned this pull request Oct 14, 2021

0.15 Release #1921

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `eyre` feature #1893

Implement `eyre` feature #1893

mejrs commented Sep 29, 2021

davidhewitt left a comment

davidhewitt Oct 1, 2021

mejrs Oct 1, 2021

davidhewitt Oct 4, 2021

davidhewitt Oct 1, 2021

mejrs Oct 1, 2021

davidhewitt Oct 4, 2021

mejrs Oct 8, 2021

davidhewitt commented Oct 1, 2021

davidhewitt Oct 4, 2021

davidhewitt Oct 4, 2021

davidhewitt Oct 4, 2021

davidhewitt left a comment

davidhewitt Oct 4, 2021

davidhewitt Oct 4, 2021

davidhewitt left a comment

mejrs commented Oct 13, 2021

		# must stay at 0.3.x for Rust 1.41 compatibility
		eyre = {version = ">= 0.4" , optional = true}

		- Add `abi3-py310` feature. [#1889](https://github.com/PyO3/pyo3/pull/1889)

	- Add `eyre` feature to convert `eyre::report` into `PyErr`. [#1893](https://github.com/PyO3/pyo3/pull/1893)
	- Add `eyre` feature to convert `eyre::Report` into `PyErr`. [#1893](https://github.com/PyO3/pyo3/pull/1893)

Implement eyre feature #1893

Implement eyre feature #1893

Conversation

mejrs commented Sep 29, 2021

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

mejrs commented Oct 13, 2021

Implement `eyre` feature #1893

Implement `eyre` feature #1893