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

#1016 Migrated INCR commands #1141

Merged
merged 17 commits into from
Oct 20, 2024
Merged

Conversation

pg30
Copy link
Contributor

@pg30 pg30 commented Oct 18, 2024

Closes #1016

@apoorvyadav1111
Copy link
Contributor

apoorvyadav1111 commented Oct 18, 2024

Hi, Thank you for making these changes.

Please add integration tests in resp, http and websocket folder. Additionally, please audit and update documentation for all the migrated commands. If this PR is still in progress, you could mark it as Draft, which will allow you to post update, ask questions, get pre-reviewed but not get Changes requested.

Edit: lint check is failing, please check.

Please reach out to me or anyone on the discord community if you have any questions.

Thanks
Apoorv

@apoorvyadav1111 apoorvyadav1111 changed the title migrated incr commands #1016 Migrated INCR commands Oct 18, 2024
@pg30 pg30 closed this Oct 18, 2024
@pg30 pg30 reopened this Oct 18, 2024
@pg30 pg30 marked this pull request as draft October 18, 2024 15:36
@pg30
Copy link
Contributor Author

pg30 commented Oct 18, 2024

Have added a new error to make the INCRBYFLOAT command consistent with redis.

	ErrInvalidFloat               = errors.New("ERR value is not a valid float")                                         // Signals that a value provided is not in a valid float format.

Have also added documentation for INCRBY and INCRBYFLOAT commands as well.

@pg30
Copy link
Contributor Author

pg30 commented Oct 18, 2024

@apoorvyadav1111 can you pre-review this if i need to do any changes.
Thanks

@apoorvyadav1111
Copy link
Contributor

Hi @pg30 , I have checked the changes as per the checklist. I see that we are still having tests in async folder. We can migrate them to resp folder, (please use any existing test file in resp folder as reference). Additionally, we need to add tests in websocket folder too.

I will look at the changes thoroughly once it checks off everything and is ready for review.

Thanks

@AshwinKul28
Copy link
Contributor

hi @pg30 you can use ErrGeneral for the error if it's not getting used at multiple evals, no need to create a separate error for this usecase. Thanks

@AshwinKul28 AshwinKul28 added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 18, 2024
@pg30
Copy link
Contributor Author

pg30 commented Oct 18, 2024

Thanks for the feedback @apoorvyadav1111 @AshwinKul28
Will make the changes accordingly

@AshwinKul28 AshwinKul28 self-requested a review October 19, 2024 06:48
@pg30 pg30 marked this pull request as ready for review October 19, 2024 11:55
@pg30
Copy link
Contributor Author

pg30 commented Oct 19, 2024

@AshwinKul28 can you review this pls

@pg30
Copy link
Contributor Author

pg30 commented Oct 20, 2024

@AshwinKul28 Http and Websockets integration tests in the PR involving big integers like math.MaxInt64 and math.MinInt64 are functionally working but not logically correct due to #1163 .
Can create a separate issue to clean up those once we fix it.

@AshwinKul28
Copy link
Contributor

Hey @pg30 Thanks a lot for this commendable effort. Changes looks good. Please verify all the docs changes once whether they are aligned with the ideal document - https://github.com/DiceDB/dice/blob/master/docs/sample_command_docs.md

You can include best practices, and additional notes if required in each of these docs.

Otherwise, the changes look great. Once docs changes are confirmed will merge the PR.

@pg30
Copy link
Contributor Author

pg30 commented Oct 20, 2024

@AshwinKul28 Thanks for reviewing the changes. I have added alternatives section for the INCR and DECR commands. Rest of the docs looks good to me. Feel free to merge the PR

Copy link
Contributor

@AshwinKul28 AshwinKul28 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@AshwinKul28 AshwinKul28 merged commit cd796f4 into DiceDB:master Oct 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration -- command Migrates current eval to a refactored eval for all protocols functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command Migration: ('INCR', 'INCRBY', 'INCRBYFLOAT', 'DECR', 'DECRBY')
3 participants