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

Add the support of the LPOS command #1681

Merged
merged 3 commits into from
Aug 19, 2023
Merged

Conversation

JoverZhang
Copy link
Contributor

Close #1528

@enjoy-binbin
Copy link
Member

i see we already have a pending #1571, what's the status over there?

@torwig
Copy link
Contributor

torwig commented Aug 18, 2023

@enjoy-binbin That issue was assigned to another person by me (almost 2 months ago) and I think @JoverZhang just took the task and did it.

@JoverZhang
Copy link
Contributor Author

Oh, Sorry, I don't know the status over there. Can I merge it?

@git-hulk
Copy link
Member

I guess it's fine since #1571 is lack active for more than one month, so it makes sense if others took over the task.

@JoverZhang
Copy link
Contributor Author

@torwig Thank you for your review and feedback. I'll make the necessary changes.

@JoverZhang
Copy link
Contributor Author

JoverZhang commented Aug 19, 2023

I did some stupid things with my GitHub App.

@PragmaTwice PragmaTwice changed the title Add the support of the LPOS command (#1528) Add the support of the LPOS command Aug 19, 2023
@git-hulk
Copy link
Member

@JoverZhang That's fine, you have done well in test coverage. You can choose to add the Go test cases only if conditions can be covered, and GoogleTest for which cannot be tested through the Go test. But now is good and you don't need to change anything.

@JoverZhang
Copy link
Contributor Author

@git-hulk Thanks, I get it now. No wonder I also think there was duplicate coverage.

Copy link
Member

@PragmaTwice PragmaTwice 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 your contribution!

LGTM at first glance.

Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

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

LGTM

@git-hulk git-hulk merged commit 695568f into apache:unstable Aug 19, 2023
enjoy-binbin added a commit to apache/kvrocks-website that referenced this pull request Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the support of the LPOS command
5 participants