R bindings#149
Conversation
ol590
left a comment
ol590
left a comment
There was a problem hiding this comment.
Code looks idiomatic and logical. However, README file needs to be more precise on what dependencies to install and in which order
tehinator
left a comment
tehinator
left a comment
There was a problem hiding this comment.
Looks good, some minor code quality comments. I think the test names here are a lot clearer than on the rust side, maybe something to look at.
|
|
||
| /// Converts a [`TestResult`] into a named R list. | ||
| /// | ||
| /// The returned list always contains a `kind` field identifying the variant: |
There was a problem hiding this comment.
maybe a name like "return format" would be more informative
| /// `data_type` must be one of `"discrete"`, `"continuous"`, or `"mixed"` (case-insensitive). | ||
| /// Returns an error for any other value. | ||
| #[extendr] | ||
| fn list_ci_tests_for(data_type: &str) -> anyhow::Result<Vec<String>> { |
There was a problem hiding this comment.
I'm not entirely certain if having a specialized version of another function is good code quality due to code duplication etc. Don't know if a better alternative is present for this case but maybe worth considering.
ol590
left a comment
ol590
left a comment
There was a problem hiding this comment.
Everything looks good. Code has been tested on Linux and runs without errors.
3 participants
Description
Proper idiomatic R bindings. Relevant files are:
crates/cir/src/rust/src/lib.rs
crates/cir/src/rust/src/util.rs
The rest of the files are all generated by extendr and hence do not need to be reviewed.
Related Issues
implements #162 #164 #143
Type of Change
Checklist
cargo fmt --all(code is formatted)cargo clippy --workspace --all-targets -- -D warnings(no warnings)cargo test --workspace(all tests pass)