-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Migrate Commands: ('SETBIT', 'GETBIT', 'BITCOUNT', 'BITPOS', 'BITFIELD', 'BITFIELD_RO') #1089
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge changes. Thanks a lot for this @vishnuchandrashekar . Left few minor comments please check and also check CI runs.
HI @vishnuchandrashekar thanks a lot for the changes. Looks good. Please rebase your branch with master to resolve conflicts. |
@vishnuchandrashekar lets ensure that the corresponding integration tests are migrated as well. cc @AshwinKul28 I have added the checklist to the PR description. Please fill the same. |
HI @vishnuchandrashekar hope you are doing well. Do you still have any blockers on this PR? please let me or others know on the Discord. |
Hey @AshwinKul28, I've moved all the tests related to BIT to the file |
Hi @vishnuchandrashekar , you can rename the file. Additionally, we do not required the async test cases anymore, if you have migrated all the tests to resp, please remove the async tests. Additionally, please add tests in websocket and http folder. Thanks |
Hey @AshwinKul28 / @apoorvyadav1111, some of the websocket and http tests seems to be failing. I am unsure as to why. Any suggestions? |
Hi @vishnuchandrashekar, I checked the lint run but there I can only see unit tests failing, Could you please share logs of failing websocket and HTTP tests on discord and tag me. |
Hi @apoorvyadav1111, apologies I meant the tests that I added. Mainly the http BITPOS tests, could you have a look at those? I'll share the logs on discord soon. Thanks. |
Yes, please share your logs on discord. Thanks |
Hi @vishnuchandrashekar are you still blocked on this issue? |
Hi @JyotinderSingh, yes I'm still facing issues. I've sent a message on discord. Currently working on updating/adding the documentation for the commands. |
HI @vishnuchandrashekar Changes look great. Please have a look at this reference doc for the documentation changes. I see a few tests are failing, consider fixing them as well. Again, thanks a lot for the commendable efforts |
HI @vishnuchandrashekar hope you are doing great. Can you please have a look at the documentation part as mentioned above? Also, is it possible to close this as early as possible? since we need to make DiceDB production ready soon for the upcoming major version release. Also more delay causes more conflicts 😭 to resolve as well. Let me know if you need any sort of help. Thanks again! |
Hey @AshwinKul28, I'll make sure to push the changes by the end of the day. Thanks for your patience. |
Hey @AshwinKul28, some of the integration tests that I've added are failing namely: HTTP:
RESP:
Websocket:
Also I am unsure as to why the build tests are failing, the tests pass on my local. |
Hey @AshwinKul28, I've fixed some of the failing test cases. I am still unsure why the RESP tests fail, would like your inputs. |
Hi @vishnuchandrashekar , #1245 is closed. Lets move ahead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.....faaantastic work! Thanks for such a great effort.
Migrates the eval functions for SETBIT, GETBIT, BITPOS, BITFIELD and BITFIELD_RO to the new eval function type independent of protocol.
Closes #1017
Checklist:
Notes:
Since the BITFIELD and BITFIELD_RO commands depend on the same function (bitfieldEvalGeneric) I have gone ahead and migrated both the commands.