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

#503: Added support for JSON.TOGGLE #546

Merged
merged 20 commits into from
Sep 16, 2024
Merged

#503: Added support for JSON.TOGGLE #546

merged 20 commits into from
Sep 16, 2024

Conversation

TheRanomial
Copy link
Contributor

@TheRanomial TheRanomial commented Sep 12, 2024 •
edited by JyotinderSingh
Loading

Fixes: #503
Added support and tests for JSON.TOGGLE command

@TheRanomial
Copy link
Contributor Author

Hi @JyotinderSingh, @lucifercr07
Can you have a look at this PR

@jujiale
Copy link
Contributor

jujiale commented Sep 12, 2024

thanks for your contribution. this pr linked #503. In case of other person is doing the same work, should notice issue with the assigned.

because json.toggle change the original json, should add integration test in tests/json_test.go
a few other comments added, please take a look thanks.

jujiale
jujiale reviewed Sep 12, 2024
View reviewed changes
internal/eval/eval.go Outdated Show resolved Hide resolved
jujiale
jujiale reviewed Sep 12, 2024
View reviewed changes
internal/eval/eval.go Outdated Show resolved Hide resolved
jujiale
jujiale reviewed Sep 12, 2024
View reviewed changes
internal/eval/eval.go Outdated Show resolved Hide resolved
@TheRanomial
Copy link
Contributor Author

Made all the changes that you requested and committed.

apoorvyadav1111
apoorvyadav1111 requested changes Sep 13, 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, the changes are good overall, left one minor comment.

internal/eval/eval.go Outdated Show resolved Hide resolved
jujiale
jujiale reviewed Sep 13, 2024
View reviewed changes
internal/eval/eval.go Outdated Show resolved Hide resolved
@TheRanomial TheRanomial changed the title Added json.toggle Added support for JSON.TOGGLE Sep 14, 2024
JyotinderSingh
JyotinderSingh requested changes Sep 14, 2024
View reviewed changes
internal/eval/eval.go Outdated Show resolved Hide resolved
@apoorvyadav1111
Copy link
Contributor

Hi @TheRanomial , @srivastava-yash, I have found a solution for our problem with expr.Set(). Please refer to PR #585 for your reference and please let me know if you find any issues in the code as well.

@JyotinderSingh
Copy link
Collaborator

Hi @TheRanomial , @srivastava-yash, I have found a solution for our problem with expr.Set(). Please refer to PR #585 for your reference and please let me know if you find any issues in the code as well.

@TheRanomial please go through the linked PR and also the comments I've left. You can modify this implementation accordingly. Please add additional test cases too (to verify behavior where multiple keys are being toggled as being discussed in other comments)

JyotinderSingh
JyotinderSingh requested changes Sep 15, 2024
View reviewed changes
Copy link
Collaborator

@JyotinderSingh JyotinderSingh 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 this PR, I have left some comments.

internal/eval/eval.go Outdated Show resolved Hide resolved
integration_tests/commands/toggle_test.go Outdated Show resolved Hide resolved
internal/eval/eval_test.go Outdated Show resolved Hide resolved
internal/eval/eval.go Outdated Show resolved Hide resolved
internal/eval/eval.go Outdated Show resolved Hide resolved
internal/eval/eval.go Outdated Show resolved Hide resolved
internal/eval/eval.go Outdated Show resolved Hide resolved
@TheRanomial
Copy link
Contributor Author

Hii @JyotinderSingh Implemented all the changes that you requested and tests are working fine.

JyotinderSingh
JyotinderSingh requested changes Sep 16, 2024
View reviewed changes
internal/eval/eval.go Outdated

if modified {
obj.Value = jsonData
store.Put(key, obj)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you resolved this without addressing it. I have fixed it with my commits though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for this. I thought that part was necessary so i didn't remove it

@JyotinderSingh JyotinderSingh mentioned this pull request Sep 16, 2024
Closed
@JyotinderSingh JyotinderSingh changed the title Added support for JSON.TOGGLE #503: Added support for JSON.TOGGLE Sep 16, 2024
@JyotinderSingh JyotinderSingh merged commit 75c0af9 into DiceDB:master Sep 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoorvyadav1111 apoorvyadav1111 apoorvyadav1111 requested changes

@JyotinderSingh JyotinderSingh JyotinderSingh requested changes

@srivastava-yash srivastava-yash srivastava-yash left review comments

@jujiale jujiale jujiale left review comments

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

Successfully merging this pull request may close these issues.

Add support for command JSON.TOGGLE
5 participants
@TheRanomial @jujiale @apoorvyadav1111 @JyotinderSingh @srivastava-yash