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

Simplify IntoInitializer #1

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

davidhewitt
Copy link

I really like the design of PyClassInitializer proposed in PyO3#683.

However, in the form proposed, I think IntoInitializer is doing too much. It's handling both what is allowed to return from #[new] as well as allowing those types to be optionally wrapped in PyResult.

This means that the use of IntoInitializer in Py<T>::new can also accept PyResult<T> as the input, which seems too generous to me. e.g. this will compile: Py::new(py, Ok(42))

This also leads to other strange API usages such as

PyResult::<i32>::Err(...).into_initializer()

I propose to simplify IntoInitializer to keep the conversion space smaller:

  • Take the error handling out of IntoInitializer
  • Re-use Into as much as possible
  • Add a new derive_utils trait IntoPyNewResult to deal with the optional wrapping in PyResult for the macro code.

With these changes, all functionality for #[new] remains intact. The coupling between IntoInitializer and PyResult would be removed, making the API neater.

@kngwyu
Copy link
Owner

kngwyu commented Jan 6, 2020

I agree that Py doesn't need to handle Result, but I have a mixed feeling for this change.

The main drawback of this change is #[new] becomes more complex. Just 1 trait is easier to understand.
I don't think this drawback is not so important.
But I think the API of Py::new is not so important, too.
It rarely appears in extension codes and actually I've never used it in my own project...

@davidhewitt
Copy link
Author

The main drawback of this change is #[new] becomes more complex.

Respectfully I disagree with this. IntoInitializer remains the public API pyo3 users need to understand for #[new], and this PR makes it simpler.

Sure, there may be one new trait IntoPyNewResult, but it is purely a codegen implementation detail that users will never need to know about.

@kngwyu kngwyu merged commit b04d0af into kngwyu:pyclass-new-layout Jan 7, 2020
@kngwyu
Copy link
Owner

kngwyu commented Jan 7, 2020

I understand, thank you!

@davidhewitt davidhewitt deleted the pyclass-new-layout branch January 7, 2020 08:05
kngwyu pushed a commit that referenced this pull request Jul 21, 2020
This prototype implements use of Py<BaseException> as the instance to
use for exception instances. These instances integrate reasonably well
with the Rust’s standard error mechanisms by implementing the `Display`
and `Error` traits. These error types can also be stored into e.g.
`failure::Fail`s or other error types as a cause of some greater error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants