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

Implement REPEAT Function #352

Merged
merged 12 commits into from
Oct 9, 2021
Merged

Implement REPEAT Function #352

merged 12 commits into from
Oct 9, 2021

Conversation

inhwa1025
Copy link
Contributor

No description provided.

@panarch panarch self-requested a review September 15, 2021 10:58
@panarch panarch added the enhancement New feature or request label Sep 15, 2021
@inhwa1025 inhwa1025 changed the title Repeatfunction Implement REPEAT Function Sep 15, 2021
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Looks really good!
I left just tiny comments here.
Let's merge this into the main soon 👍

src/tests/function/repeat.rs Outdated Show resolved Hide resolved
Comment on lines 585 to 587
Nullable::Null => {
return Ok(Evaluated::from(Value::Null));
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's just have a single more test case.
Precisely, this Nullable::Null is not tested by current repeat integration test

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2021

Codecov Report

Merging #352 (6cc1715) into main (e0a67a9) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
+ Coverage   91.71%   91.73%   +0.02%     
==========================================
  Files         138      139       +1     
  Lines        8434     8460      +26     
==========================================
+ Hits         7735     7761      +26     
  Misses        699      699              
Impacted Files Coverage Δ
src/tests/mod.rs 100.00% <ø> (ø)
src/executor/evaluate/mod.rs 95.31% <100.00%> (+0.06%) ⬆️
src/tests/function/repeat.rs 100.00% <100.00%> (ø)
src/translate/function.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0a67a9...6cc1715. Read the comment docs.

Function::Repeat { expr, num } => {
let expr = eval_to_str!(expr);
let num = eval_to_integer!(num);
Ok(Value::Str(expr.repeat(num.try_into().unwrap())))
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to use unwrap().
num.try_into()? will work.

use crate::*;

test_case!(repeat, async move {
//use Value::{Null, Str, I64};
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this :)

@panarch panarch self-requested a review October 9, 2021 16:17
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Awesome 👍 👍 👍
Good start!
All looks good, merging!

@panarch panarch merged commit 1b6b4b8 into gluesql:main Oct 9, 2021
@inhwa1025 inhwa1025 deleted the REPEATfunction branch October 24, 2021 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants