Add option to use try from in extendr macro #222
Conversation
andy-thomason
requested review from
clauswilke,
Ilia-Kosenkov and
yutannihilation
extendr-api/tests/extendr_macro.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn happy_path() { |
There was a problem hiding this comment.
Could you use a more descriptive function name? E.g. tests_without_error()?
| @@ -1,6 +1,6 @@ | |||
| //! R object handling. | |||
| //! | |||
| //! See. https://cran.r-project.org/doc/manuals/R-exts.html | |||
| //! See. <https://cran.r-project.org/doc/manuals/R-exts.html> | |||
| //! | |||
| //! Fundamental principals: | |||
| } | ||
|
|
||
| // Win32 does not support catch_unwind. | ||
| #[cfg(not(target_arch = "x86"))] |
There was a problem hiding this comment.
Would it make sense to make this test Windows specific? Or is x86 specific to Windows?
There was a problem hiding this comment.
I think that the problem (catch_unwind not supported) is x86 specific, rather than windows,
but Windows (XP) is currently our only x86 target.
Getting the tests to run green is a priority here, we could refine later.
extendr-api/tests/extendr_macro.rs
Outdated
| // Win32 does not support catch_unwind. | ||
| #[cfg(not(target_arch = "x86"))] | ||
| #[test] | ||
| fn sad_path() { |
There was a problem hiding this comment.
See above. E.g., tests_with_errors().
| -1 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
a test_option_bool as well?
There was a problem hiding this comment.
I'll merge this into main. Feel free to add more tests.
| // NA input. | ||
| assert_eq!(new_owned(wrap__test_option_f64(r!(NA_INTEGER).get())), r!(-1.0)); | ||
| } | ||
| } |
There was a problem hiding this comment.
if adding test_option_bool above: a series for logical
|
looks nice -- i've commented on one test that snuck away this time, but adding it could be (edit perhaps was intended to be) work for others as you mentioned in #219. |
| assert_eq!(new_owned(wrap__test_i32(r!(1).get())), r!(1)); | ||
|
|
||
| // i32 takes any numeric. | ||
| assert_eq!(new_owned(wrap__test_i32(r!(1.0).get())), r!(1)); |
There was a problem hiding this comment.
I am not sure what 'takes any numeric' means here. What will happen if we pass 1.5?
What would be the behavior?
If the value is somehow truncated/converted to integer space, this may be an issue, and extremely hard to debug.
Same applies to Option<i32>
If this is OK, may I then suggest to add another option, like #[extendr(use_strict_types = true)] or #[extendr(vctrs_compat = true)] to enable strict type checks?
There was a problem hiding this comment.
If you feel like doing so, we should replace the "as" in impl_try_from_scalar! with TryFrom.
This would also range-check small numbers.
I'm going to close this ASAP so that more PRs can follow.
There was a problem hiding this comment.
We now have a template for adding other options, so suggestions like this are welcome.
There was a problem hiding this comment.
Just wanted to point out that I think strict type checks should be the default, not optional. If somebody writes i32 as their input type we shouldn't silently round from 1.5 to 1, we should throw a runtime error.
There was a problem hiding this comment.
I fully support this. My preferred solution would be something similar to {vctrs} behavior, which should at least be available as an option.
|
|
||
|
|
||
| // Matching integer. | ||
| assert_eq!(new_owned(wrap__test_option_i16(r!(1).get())), r!(1)); |
There was a problem hiding this comment.
Without having a deep understanding of what #[extendr(use_try_from = true)] produces, I would suggest adding tests for int values outside of i16 value space.
There was a problem hiding this comment.
See the comment above about replacing the "as".
This PR adds an option:
This is a prelude to supporting the try_from error handling formally.
It should have no effect on the main branch and so, we may merge this to enable others to contribute
to writing conversions.
We may need the failing conversion to give a useful error message by using to_string() on the returned
error.
We should then write integration tests to check these messages in R.
Note that Option gives None if T is a NA value, not NULL.
There is another wrapper
Nullablewhich handles NULLs.