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

Add LSET command #274

Merged
merged 14 commits into from
Apr 23, 2024
Merged

Add LSET command #274

merged 14 commits into from
Apr 23, 2024

Conversation

tisilent
Copy link
Contributor

Add #272.

@tisilent
LSET draft.
56621a5
@tisilent
Add case tests.
96cdf66 
Add doc.
Fix TryBytesToInt.
@tisilent
Copy link
Contributor Author

@TalZaccai I will adjust the variable names later...

PaulusParssinen
PaulusParssinen suggested changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@PaulusParssinen PaulusParssinen left a comment

Choose a reason for hiding this comment

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

Few nits from me, we don't need to allocate the constant errors

libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
@TalZaccai TalZaccai self-assigned this Apr 12, 2024
@TalZaccai TalZaccai linked an issue Apr 12, 2024 that may be closed by this pull request
API Coverage - Implement LSET #272
Closed
@TalZaccai TalZaccai added the API label Apr 12, 2024
tisilent and others added 5 commits April 13, 2024 20:50
@tisilent @PaulusParssinen
Update libs/server/Objects/List/ListObjectImpl.cs
2874133 
Co-authored-by: Paulus Pärssinen <paulus.parssinen@gmail.com>
@tisilent @PaulusParssinen
Update libs/server/Objects/List/ListObjectImpl.cs
70cdbc5 
Co-authored-by: Paulus Pärssinen <paulus.parssinen@gmail.com>
@tisilent @PaulusParssinen
Update libs/server/Objects/List/ListObjectImpl.cs
7643400 
Co-authored-by: Paulus Pärssinen <paulus.parssinen@gmail.com>
@tisilent
Merge branch 'main' into add-LSET-command
8960828
@tisilent
Rename to _in_* , _out_
7ed643b
@tisilent
Copy link
Contributor Author

@TalZaccai I have fixed the issue in NumUtil, where would it be better to add test cases.

@TalZaccai
Copy link
Contributor

@TalZaccai I have fixed the issue in NumUtil, where would it be better to add test cases.

Good catch! Thanks!

TalZaccai
TalZaccai requested changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

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

Nice work! Added some comments.

libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
test/Garnet.test/RespListTests.cs Show resolved Hide resolved
@tisilent
Copy link
Contributor Author

@TalZaccai
Hi.arguments error
Garnet:
ERR wrong number of arguments for 'LSET'
Redis:
ERR wrong number of arguments for command
...

@tisilent tisilent requested a review from TalZaccai April 19, 2024 05:48
@TalZaccai
Copy link
Contributor

TalZaccai commented Apr 23, 2024 •
edited
Loading

@TalZaccai Hi.arguments error Garnet: ERR wrong number of arguments for 'LSET' Redis: ERR wrong number of arguments for command ...

I think it varies by the version of Redis you're using...
This is what I'm seeing locally:
image

TalZaccai
TalZaccai approved these changes Apr 23, 2024
View reviewed changes
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/List/ListObjectImpl.cs Outdated Show resolved Hide resolved
@TalZaccai TalZaccai merged commit df94b6d into microsoft:main Apr 23, 2024
21 checks passed
@tisilent tisilent deleted the add-LSET-command branch April 24, 2024 00:51
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PaulusParssinen PaulusParssinen PaulusParssinen requested changes

@TalZaccai TalZaccai TalZaccai approved these changes

Assignees

@TalZaccai TalZaccai

Labels
API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API Coverage - Implement LSET
3 participants
@tisilent @TalZaccai @PaulusParssinen