Better list macro #303
Better list macro #303
Conversation
andy-thomason
force-pushed
the
better-list-macro
branch
from
5f0b0b1
to
2cbf54f
Compare
andy-thomason
requested review from
clauswilke,
yutannihilation and
Ilia-Kosenkov
extendr-api/src/wrapper/list.rs
Outdated
| let mut list = List::from_values(values); | ||
| list.set_names(names).unwrap(); | ||
| list |
There was a problem hiding this comment.
So what happens if names length does not equal to that of values? panic?
There was a problem hiding this comment.
Good catch. I think this is a problem. Does the list! macro have the same problem?
There was a problem hiding this comment.
The List macro should be guaranteed to make them the same length.
We probably should at least check the lengths in this function.
set_names is a bit dangerous as R may have some rules that it enforces.
I'm hoping that we will use the list! macro more than from_values_and_names in the future.
There was a problem hiding this comment.
The question is, in this particular function, what happens if we provide 5 values and 2 names? There is no simple logic for generating 5 names from 2 strings, and I think it is important to understand.
andy-thomason
force-pushed
the
better-list-macro
branch
from
414c6d4
to
26d05f2
Compare
| let names: Vec<proc_macro2::TokenStream> = | ||
| nv.iter().map(|(n, _v)| quote!( #n )).collect(); | ||
| TokenStream::from(quote!( | ||
| extendr_api::List::from_names_and_values(&[# ( #names ),*], &[# ( #values ),*]).unwrap() |
There was a problem hiding this comment.
It seems the advantage of the list macro is convenience, but at the cost of a panic if the input has the wrong format. I suggest you write this somewhere in the documentation.
There was a problem hiding this comment.
We should probably return an error.
This means we must use list!(...)?
There was a problem hiding this comment.
At the moment, the only way it will unwrap and error is if it is not a List (always false) or the lengths do not match (always false).
So we should probably ignore this.
| @@ -21,3 +21,20 @@ impl syn::parse::Parse for Pairs { | |||
| Ok(res) | |||
| } | |||
| } | |||
|
|
|||
| impl Pairs { | |||
There was a problem hiding this comment.
Could you add some documentation here explaining what this is for?
| @@ -113,6 +114,15 @@ pub fn pairlist(item: TokenStream) -> TokenStream { | |||
| pairlist::pairlist(item) | |||
| } | |||
|
|
|||
| /// Create a List R object from a list of name-value pairs. | |||
There was a problem hiding this comment.
I think it would be good to mention here the potential for a panic when the input data is of the wrong format, and point users to the safer function interface instead.
extendr-api/src/wrapper/list.rs
Outdated
| @@ -63,6 +65,22 @@ impl List { | |||
| res.set_names(names).unwrap().as_list().unwrap() | |||
| } | |||
|
|
|||
| /// Build a list using spearate names and values iterators. | |||
| /// Used by the list! macro. | |||
There was a problem hiding this comment.
Would it make sense to add a use case demonstration here?
|
Thansk for all the feedback. I'm going to merge it anyway for fear of it getting stale. We should absolutely not expect it to panic, but R may have other ideas (maybe reserved names and such like). We could also have |
andy-thomason
force-pushed
the
better-list-macro
branch
from
ad5b980
to
50b28f5
Compare
This PR makes a procedural version of the list! macro which now returns the List type.
There are also a few structural changes to List.
As
Robjis now a wrapper forSEXPwe can express lists as an array ofRobjtypes.I have also added a more efficient method to create named lists.