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.

Sign up for GitHub

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

Jump to bottom

[Function] Implement REPLACE function #1266

Merged
merged 7 commits into from
Jul 12, 2023
Merged

[Function] Implement REPLACE function #1266

merged 7 commits into from
Jul 12, 2023

Conversation

ChobobDev
Copy link
Contributor

resolves #1146

@ChobobDev ChobobDev marked this pull request as ready for review July 10, 2023 15:26
@coveralls
Copy link

coveralls commented Jul 10, 2023 •
edited

Pull Request Test Coverage Report for Build 5521354831

  • 81 of 91 (89.01%) changed or added relevant lines in 5 files are covered.
  • 7 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.01%) to 98.724%
Changes Missing Coverage Covered Lines Changed/Added Lines %
test-suite/src/function/replace.rs 41 51 80.39%
Files with Coverage Reduction New Missed Lines %
test-suite/src/data_type/list.rs 1 98.1%
test-suite/src/data_type/map.rs 1 97.44%
core/src/data/value/mod.rs 5 99.62%
Totals Coverage Status
Change from base Build 5498961825: -0.01%
Covered Lines: 44728
Relevant Lines: 45306
💛 - Coveralls

@ChobobDev
Copy link
Contributor Author

I think I am ready for review!

@panarch panarch added the enhancement New feature or request label Jul 10, 2023
zmrdltl
zmrdltl approved these changes Jul 11, 2023
View reviewed changes
Copy link
Collaborator

@zmrdltl zmrdltl left a comment

Choose a reason for hiding this comment

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

Thanks for adding the function, now GlueSQL has a new feature!

devgony
devgony requested changes Jul 11, 2023
View reviewed changes
test-suite/src/function/replace.rs Outdated Show resolved Hide resolved
@ChobobDev
Copy link
Contributor Author

ChobobDev commented Jul 11, 2023 •
edited

I have updated the test-suites as guided, if you have any doubts about following changes, please feel free to review my work!
Thanks in advance

@ChobobDev ChobobDev requested a review from devgony July 12, 2023 05:09
devgony
devgony approved these changes Jul 12, 2023
View reviewed changes
Copy link
Collaborator

@devgony devgony left a comment

Choose a reason for hiding this comment

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

Thank you for writing fine-grained test cases 👍🏼

panarch
panarch approved these changes Jul 12, 2023
View reviewed changes
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 all great!
Also thanks a lot for adding well documented integration tests.

@panarch panarch merged commit 2aa97ce into gluesql:main Jul 12, 2023
9 checks passed
@ChobobDev ChobobDev deleted the feat/replace branch July 12, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panarch panarch panarch approved these changes

@zmrdltl zmrdltl zmrdltl approved these changes

@devgony devgony devgony approved these changes

@ever0de ever0de Awaiting requested review from ever0de

Assignees

@ChobobDev ChobobDev

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support REPLACE function
5 participants
@ChobobDev @coveralls @panarch @zmrdltl @devgony