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

Python error handling #215

Merged
merged 14 commits into from
Aug 10, 2020
Merged

Python error handling #215

merged 14 commits into from
Aug 10, 2020

Conversation

weslenng
Copy link

@weslenng weslenng commented Aug 9, 2020

This PR add support for Python error handling

Closes #202
Closes #206

@tarikeshaq tarikeshaq requested review from tarikeshaq and rfk August 9, 2020 16:05
Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

Thanks, @weslenng!! This is a really good start! I dropped a couple of comments on some of your concerns.

Also, heads up to keep one eye on #214 as that refactors the Python bits. Let me know if you have any other concerns!

examples/arithmetic/src/lib.rs Outdated Show resolved Hide resolved
examples/arithmetic/tests/bindings/test_arithmetic.swift Outdated Show resolved Hide resolved
examples/arithmetic/tests/bindings/test_arithmetic.kts Outdated Show resolved Hide resolved
examples/arithmetic/tests/bindings/test_arithmetic.kts Outdated Show resolved Hide resolved
examples/arithmetic/tests/test_generated_bindings.rs Outdated Show resolved Hide resolved
uniffi_bindgen/src/bindings/python/templates/wrapper.py Outdated Show resolved Hide resolved
uniffi_bindgen/src/bindings/python/templates/wrapper.py Outdated Show resolved Hide resolved
@weslenng weslenng changed the title [WIP] Python error handling Python error handling Aug 9, 2020
@weslenng weslenng requested a review from tarikeshaq August 9, 2020 21:05
Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

This is almost there! Awesome job! 🚀

Dropped a couple of comments, mainly requesting changes for the error code matching and freeing but this is looking good!

Copy link
Collaborator

@rfk rfk left a comment

Choose a reason for hiding this comment

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

I like how this is coming together! I think you can simplify the code by using a stateless helper function rather than the stateful RustErrorHelper method, see my comments inline below.

@weslenng
Copy link
Author

weslenng commented Aug 10, 2020

Thanks for the feedback, it's probably fine now!

@weslenng weslenng requested review from rfk and tarikeshaq August 10, 2020 20:59
Copy link
Collaborator

@rfk rfk 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 left a couple of very minor comments below, but this looks really good overall.

(I'm going to wait to hear from @linacambridge over in #221 to make sure we don't generate merge conflicts with her work, before merging this)

try {
sub(0, 2)
throw RuntimeException("Should have thrown a IntegerOverflow exception!")
} catch (e: ArithmeticErrorException) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In swift and python we explicitly test that this was the IntegerOverflow variant, but here we test only that it was an ArithmeticError. Does it work if we catch ArithmeticErrorException.IntegerOverflow here like the other languages do?

(I note that the todolist example has a similar problem, but we don't need to fix that here)

_UniFFILib.{{ func.ffi_func().name() }}({% call _arg_list_ffi_call(func.arguments()) -%})
{%- match func.throws() -%}
{%- when Some with (e) -%}
rust_call_with_error({{ e|class_name_py }},_UniFFILib.{{ func.ffi_func().name() }},{% call _arg_list_ffi_call(func) -%})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, I really like how this worked out in practice 👍

Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

LGTM! Amazing job @weslenng!

We probably can use better tests for our errors overall. I'm realizing that we don't have tests with multiple error types 😢

Hopefully, that can slide into #176 but we can live with this for now!

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.

Code generation failing in Python Add error handling for python
3 participants