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

#1193: Add type check for string compatibility in evalAPPEND function #1237

Merged
merged 5 commits into from
Nov 12, 2024
Merged

#1193: Add type check for string compatibility in evalAPPEND function #1237

merged 5 commits into from
Nov 12, 2024

Conversation

shashi-sah2003
Copy link
Contributor

fixes #1193
added type check for string compatibility as it was giving panic issue with append is used after ZADD command

@shashi-sah2003
Copy link
Contributor Author

shashi-sah2003 commented Nov 5, 2024 •
edited
Loading

@apoorvyadav1111 pls look at this PR and review it and also let me know if there is anything more I need to check with append command

EDIT: I'm working on why this is failing on tests

@apoorvyadav1111
Copy link
Contributor

Hi @shashi-sah2003 , any update on failing tests? Please reach out to use on discord if you need any clarifications.

@shashi-sah2003
Copy link
Contributor Author

Hi @shashi-sah2003 , any update on failing tests? Please reach out to use on discord if you need any clarifications.

hey @apoorvyadav1111 apologies for delay. I have added the logic for append command such that it is compatible with sortedset.
pls let me know if there are any more changes you want me to do.
thanks

@shashi-sah2003 shashi-sah2003 mentioned this pull request Nov 9, 2024
Draft
apoorvyadav1111
apoorvyadav1111 requested changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@apoorvyadav1111 apoorvyadav1111 left a comment

Choose a reason for hiding this comment

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

Hi @shashi-sah2003 , The changes look good. Could you please add tests to validate this behavior? This will ensure any future modifications do not alter this behavior.

Thanks

apoorvyadav1111
apoorvyadav1111 approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@apoorvyadav1111 apoorvyadav1111 left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests. Approved

@apoorvyadav1111 apoorvyadav1111 changed the title Add type check for string compatibility in evalAPPEND function #1193: Add type check for string compatibility in evalAPPEND function Nov 12, 2024
@apoorvyadav1111 apoorvyadav1111 merged commit 51ccd04 into DiceDB:master Nov 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoorvyadav1111 apoorvyadav1111 apoorvyadav1111 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: Append to support other datatypes
2 participants
@shashi-sah2003 @apoorvyadav1111