#1020 Migrate SADD, SREM, SCARD, SMEMBERS command to store_eval

[sahoss]

Description
Migrates the eval functions for SADD, SREM, SMEMBERS, SCOUNT to the new eval function type (#1020)

Checklist

• Migrated the evalXXX function with the latest definition
• Update or add unit tests for the new implementation.
• All unit tests pass successfully.
• Ensure all integration tests pass successfully.
• Ensure integration tests are added for the migrated command on multi-threaded resp server.
• Move Integration tests for the respective commands under the RESP integration tests directory from Async directory
• Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

[AshwinKul28]

HI @sahoss changes look good. 🚀 left a minor comment.

Please pull the latest from the master to resolve the linter issue. Thanks

[AshwinKul28]

HI @sahoss Thanks for the changes, LGTM! Please add migration for 'SMEMBERS', 'SCARD' also in the same PR.

[sahoss]

@AshwinKul28 added the remaining commands. however, i still see lint errors locally and i have merged latest master into my branch. its not from my changes. ok to ignore?

[AshwinKul28]

Hi @sahoss thanks a lot for the changes. Can you please add unit tests for each eval? It's not present already but you can add new unit tests for each command in the eval_test.go Also please rebase it with master.

[soumya-codes]

@sahoss lets ensure that the corresponding integration tests are migrated as well. cc @AshwinKul28

[sahoss]

@AshwinKul28 / @soumya-codes - sure. it will take me until this weekend potentially to make those changes. just a heads up

[soumya-codes]

Just FYI, we need to add the tests for the migrated commands under integration_tests/commands/resp

I have added a new item to checklist in the PR description. Please tick the same once the integration tests are added.
Please let me or Ashwin know if you have any doubts/concern.

[AshwinKul28]

HI @sahoss Hope you're doing well, Do you still have any blockers on this? Let me know and we can resolve them quickly and try to merge the PR. thanks a lot for all the efforts.

[sahoss]

@AshwinKul28 i had a question for you here #1068 (comment).

i am working in parallel to refresh the documentation.

[sahoss]

doc refresh pr @ DiceDB/docs#116

[AshwinKul28]

Thanks @sahoss for the documentation refresh. I have left a single comment on the docs PR! Otherwise, is everything related to the migration sorted, or are HTTP tests failing?

[sahoss]

@AshwinKul28 everything should be passing now. i have addressed the documentation PR comment

[apoorvyadav1111]

Thanks for this fantastic effort. Left few comments. In addition to resp, please add test cases in websocket and HTTP. Thanks.

[AshwinKul28]

@sahoss Please address above requested changes by Apoorv, then we are good to merge. thanks

[apoorvyadav1111]

Approving, looks good to me

[AshwinKul28]

Looks good to me.

Thanks a lot @sahoss for the efforts and thanks @apoorvyadav1111 for the last minute conflict resolutions.

[AshwinKul28]

@apoorvyadav1111 still 1 conflict in the store eval file :(

[sahoss]

@apoorvyadav1111 still 1 conflict in the store eval file :(

i have fixed the conflict.

thanks @apoorvyadav1111 @AshwinKul28 for your detailed reviews. appreciate your time and effort on this pr.

[apoorvyadav1111]

Hi @sahoss , apologies for commiting and reverting the deque test change. We recently migrated to this package and would not want to revert the efforts and reopen another issue to migrate again. I fixed the issue, it was a call to old assert package, which could have been reverted as part of recent merge.

@AshwinKul28 Thanks for your review. I am merging this as the changes after your review are minimal and we don't want another conflict. Thank you both for making this change into main.