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

Make Rfloat, Rint, and friends ToVectorValue. #593

Merged
merged 17 commits into from
Oct 2, 2023
Merged

Conversation

CGMossa
Copy link
Member

@CGMossa CGMossa commented Aug 25, 2023

Fixes #592

This PR does two things:

  • Makes Rints, &Rints, Rfloat, and &Rfloat as ToVectorValue, as these are wrapped Rust types
    that are safe to assume would translate to R. Moreover Rbool and &Rbool were already ToVectorValue,
    so it could have been a slight oversight

  • We are increasingly promoting Integers, Logicals, Doubles, and Complexes as the better way
    to extract vectors from R. Thus to make these more easier to work with, I've added TryFrom<Vec<RustNative>> for all of them. This was previously accomplished with from_values,
    but I think these are intuitive interface for these as well.

@CGMossa
Copy link
Member Author

CGMossa commented Aug 25, 2023

Alright, except some more polish, I believe this is ready for review. Pinging @JosiahParry, @daniellga and @Ilia-Kosenkov for reviews / opinions.

@JosiahParry
Copy link
Contributor

@CGMossa thank you so much for making this PR! My naive eyes think its looks okay but im not even close to an expert here :P

extendr-api/src/wrapper/logicals.rs Outdated Show resolved Hide resolved
extendr-api/src/wrapper/logicals.rs Outdated Show resolved Hide resolved
extendr-api/src/wrapper/matrix.rs Show resolved Hide resolved
extendr-api/src/wrapper/matrix.rs Show resolved Hide resolved
extendr-api/src/wrapper/integers.rs Outdated Show resolved Hide resolved
extendr-api/src/wrapper/doubles.rs Show resolved Hide resolved
extendr-api/src/wrapper/complexes.rs Show resolved Hide resolved
@JosiahParry
Copy link
Contributor

@CGMossa thanks for adding extra tests and comments! Looks good to my novice eyes!

@JosiahParry
Copy link
Contributor

@CGMossa there's one cargo fmt ci issue :/ Otherwise looks good and quite handy!

Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

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

Run cargo fmt

@CGMossa CGMossa merged commit 07914b8 into master Oct 2, 2023
14 of 15 checks passed
@CGMossa CGMossa deleted the tovectorvalue_r_types branch October 2, 2023 16:23
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.

ToVectorValue not implemented for Rfloat
4 participants