Get SymPair to return Option<Symbol>
#225
Conversation
| if let Some(n) = n { | ||
| unsafe { Rf_defineVar(n.get(), v.get(), robj.get()) } | ||
| } |
There was a problem hiding this comment.
One thing not very obvious is what we should do when the user try to create an environment from a pairlist containing unnamed element. Currently it's just ignored. Should we raise an error?
There was a problem hiding this comment.
What is ignored? The unnamed element only, or the entire call? And if you triggered an error, would it be compile time or runtime?
There was a problem hiding this comment.
The unnamed element only, or the entire call?
Currently this ignores the unnamed elements only.
And if you triggered an error, would it be compile time or runtime?
Runtime.
There was a problem hiding this comment.
It's unclear to me. Usually, my philosophy is that code should use meaningful fallback behavior on empty input, so that would argue against raising an error. I have encountered so many cases where code would complain about NA or NULL input instead of silently ignoring it, and then I have to special-case these situations, and the code I'm calling is also special-casing, and it just doesn't make sense.
I assume the counter-argument is that silently ignoring invalid input can create difficult-to-find bugs.
There was a problem hiding this comment.
I'm getting inclined to raise an error, rather than handling the invalid inputs gracefully. I think this operation is for special cases when there's a lot of inputs generated programmatically, as the example shows. For more dynamic inputs, users can process the elements one by one with Environment::set_local() by themselves.
/// let names_and_values = (0..100).map(|i| (format!("n{}", i), i));
/// let mut env = Environment::from_pairs(global_env(), names_and_values);
/// assert_eq!(env.len(), 100);
|
I have reviewed this but don't feel qualified to assess whether the change from |
…pair-return-option
Fix #195
This pull request is to remove the necessity to use unbound value to represent the absence of the name. I decided to follow Andy's comment. Instead, users can use
""(empty string).This is consistent with the behavior of R (c.f. #195 (comment), #195 (comment))
While this is a breaking change in that
SymPair::sym_pair()now returns(Option<Robj>, Robj)instead of(Robj, Robj), I expect this doesn't affect the existing users much as this is rather an expert-only feature at the moment.