-
Notifications
You must be signed in to change notification settings - Fork 209
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
Support user level sql function #1095
Conversation
Pull Request Test Coverage Report for Build 4677170930
💛 - Coveralls |
7cabf7f
to
58b6417
Compare
0cee14f
to
0522471
Compare
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.
Wow, now it's really close to the completion!
With some comments, it would be nice that we can add a few more tests to reach uncovered lines.
ref. you can ignore await
uncovered lines which we cannot make it work.
https://coveralls.io/builds/57904316
8f3c4f5
to
b225b2a
Compare
core/src/data/mod.rs
Outdated
@@ -1,4 +1,5 @@ | |||
mod bigdecimal_ext; | |||
pub mod function; |
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.
pub mod function; | |
mod function; |
expected_minimum: usize, | ||
expected_maximum: usize, |
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.
expected_minimum: usize, | |
expected_maximum: usize, | |
expected_range: Range<usize> |
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.
@ever0de current custom function does not support open range so it would be better to use expected_minimum
& expected_maximum
.
Range
can describe (3..)
or (..=4)
like open ranges that are not supported by our custom function args
@ever0de , finally i done a rebase. I apply reviews. |
Could you please confirm and apply CI? |
@panarch, i fixed it |
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.
Ok now it's really the time to ship this awesome new feature to the main!
Looks all great 👍 👍 👍
Thanks a lot!
Linked to
#660
TODO
Example