#1019 Migrated LPUSH, RPUSH, LPOP, RPOP, LLEN - Githubissues

DiceDB / dice

DiceDB is a redis-compliant, reactive, scalable, highly-available, unified cache optimized for modern hardware.

https://dicedb.io/

Other

6.86k stars 1.09k forks source link

#1019 Migrated LPUSH, RPUSH, LPOP, RPOP, LLEN #1181

Closed Aditya-Chowdhary closed 1 week ago

Aditya-Chowdhary commented 1 month ago

fixes #1019

Edit: Now ready for review Currently draft PR as updating integration tests is remaining. Remaining tasks are completed.

For migration commands, please consider this a template on how to tackle the issue. I have divided the tasks into smaller goals and made an individual commit.	Condition	Return Value
Migrating eval to store eval
Modify unit test with new response format
Migrate integration tests from async to resp
Add/update integration in websocket
Add/update integration in HTTP
Ensure all integration test pass
Update/Add docs for the commands
Lint checks

[x] Migrated the evalXXX function with the latest definition
[x] Update or add unit tests for the new implementation.
[x] All unit tests pass successfully.
[x] Ensure all integration tests pass successfully (Resp/http/websocket)
[x] Move Integration tests for the respective commands under the RESP integration tests directory from the Async directory
[x] Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

Aditya-Chowdhary commented 4 weeks ago

@AshwinKul28 I have made the requested changes. Several of the ones you commented on did not return an error, so I have left those as clientio.NIL for now, please let me know if I should change those ones as well.

https://github.com/DiceDB/dice/pull/1181/files#r1818149611 https://github.com/DiceDB/dice/pull/1181/files#r1818150264 https://github.com/DiceDB/dice/pull/1181/files#r1818150407

AshwinKul28 commented 3 weeks ago

HI @Aditya-Chowdhary thanks again for the commendable efforts. The changes look great.

Please rebase your branch with the master and I have added a checklist in the PR description, go through it and complete those tasks. (Most of them already you have taken care :D)

For the documentation of LPUSH, RPUSH, RPOP commands we need a refresher based on this sample doc such as example descriptions, notes and best practices if required.

Aditya-Chowdhary commented 2 weeks ago

Apologies for the delay in this, it's been a busy couple of days, I'll rebase the changes tomorrow(7/11).

Aditya-Chowdhary commented 2 weeks ago

@JyotinderSingh @AshwinKul28 I have completed the rebase and checked the checklist.

JyotinderSingh commented 2 weeks ago

There is an integration test failure, could you please take a look?

Aditya-Chowdhary commented 2 weeks ago

@JyotinderSingh Have fixed all merge conflicts + test failures Edit: Grammar

apoorvyadav1111 commented 1 week ago

Thanks for taking up this issue and closing it.