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

Allow other Result types in #[pyfunction] #1118

Merged
merged 10 commits into from Aug 29, 2020
Merged

Allow other Result types in #[pyfunction] #1118

merged 10 commits into from Aug 29, 2020

Conversation

marioortizmanero
Copy link
Contributor

@marioortizmanero marioortizmanero commented Aug 26, 2020

Thank you for contributing to pyo3!

Please consider adding the following to your pull request:

  • an entry in CHANGELOG.md
  • docs to all new functions and / or detail in the guide
  • tests for all new or changed functions

Be aware the CI pipeline will check your pull request for the following:

  • Rust tests (Just cargo test or make test if you need to test examples)
  • Rust lints (make clippy)
  • Rust formatting (cargo fmt)
  • Python formatting (black . --check. You can install black with pip install black)
  • Compatibility with all supported Python versions for all examples. This uses tox; you can do run it using make test_py.

You can run a similar set of checks as the CI pipeline using make test.


I've implemented #1106, see that issue for a description. It was quite simple but I haven't touched this codebase before so please let me know if anything can be improved.

I included a couple tests, one where it works correctly, and another where it fails to compile because from From<MyErr> for PyErr isn't implemented. What I would like to know before this is merged:

  • Is it possible to improve the error message that appears when it fails to compile? See the tests/ui/invalid_result_conversion.stderr for the current error message.
  • I still have to include this in the documentation. Any pointers would be appreciated for that, since I don't know where this should go in the book.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I have some minor concerns but it looks that it's nicely fixed 👍

tests/test_result_conversion.rs Outdated Show resolved Hide resolved
tests/ui/invalid_result_conversion.rs Outdated Show resolved Hide resolved
tests/test_result_conversion.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks like this was even easier than expected 😄

As for the guide: maybe in this section for now? https://pyo3.rs/v0.11.1/conversions.html#returning-rust-values-to-python

I plan to make a bunch of PRs to improve the guide soon anyway, so as long as this functionality is mentioned in the guide I'll probably move it around if necessary ;)

@marioortizmanero
Copy link
Contributor Author

Great, I've made all the suggested fixes now, including mentioning this in the docs.

@davidhewitt
Copy link
Member

Thanks, LGTM! Regarding the UI test - I don't think we'll easily improve the error message (that's pretty much part of the compiler), so I'm just going to merge for now and maybe we can improve this later.

@davidhewitt davidhewitt merged commit 608aea7 into PyO3:master Aug 29, 2020
@marioortizmanero marioortizmanero deleted the convert-result branch August 29, 2020 08:02
@marioortizmanero
Copy link
Contributor Author

Thanks! I'm glad I could contribute to PyO3. Cheers :)

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.

None yet

3 participants