Skip to content

Allow other Result types in #[pyfunction] - #1118

Merged
davidhewitt merged 10 commits into
PyO3:masterfrom
marioortizmanero:convert-result
Aug 29, 2020
Merged

Allow other Result types in #[pyfunction]#1118
davidhewitt merged 10 commits into
PyO3:masterfrom
marioortizmanero:convert-result

Conversation

@marioortizmanero

@marioortizmanero marioortizmanero commented Aug 26, 2020

Copy link
Copy Markdown

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.

@kngwyu kngwyu left a comment

Copy link
Copy Markdown
Member

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 👍

Comment thread tests/test_result_conversion.rs Outdated
Comment thread tests/test_result_conversion.rs Outdated
Comment thread tests/test_result_conversion.rs Outdated
Comment thread tests/ui/invalid_result_conversion.rs Outdated
Comment thread tests/test_result_conversion.rs Outdated

@davidhewitt davidhewitt left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Author

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

@davidhewitt

Copy link
Copy Markdown
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
Copy Markdown
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.

3 participants