-
Notifications
You must be signed in to change notification settings - Fork 210
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
Implement Rand function #1063
Implement Rand function #1063
Conversation
Pull Request Test Coverage Report for Build 3932940113
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some very nit comments.
Everything else looks nice.
Thanks a lot :)
also thanks for adding rand
support to ast_builder.
let expr = match expr { | ||
Some(v) => Some(eval(v)?), | ||
None => None, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. there is eval_opt
sugar closure function. so you can use
let expr = eval_opt(expr.as_ref())?;
pub fn rand<'a, T: Into<ExprNode<'a>>>(expr: T) -> ExprNode<'a> { | ||
ExprNode::Function(Box::new(FunctionNode::Rand(Some(expr.into())))) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, using ast builder we cannot use random function without seed.
It would be better to take Option<ExprNode<'a>>
rather than Into<ExprNode<'a>>
.
Like.. rtrim
and ltrim
.
fyi.
For now, when we use Option<ExprNode<'a>>
, then Into
is not supported yet.
Thanks for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good. Thanks a lot 👍
resolve #593