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

Add the support of the json.arrappend command #1837

Merged
merged 7 commits into from
Oct 20, 2023
Merged

Add the support of the json.arrappend command #1837

merged 7 commits into from
Oct 20, 2023

Conversation

lieck
Copy link
Contributor

@lieck lieck commented Oct 19, 2023

This PR adds support for JSON.ARRAPEND command.
Closes #1806

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

General looks good to me, just some nits.

src/types/json.h Outdated Show resolved Hide resolved
src/types/json.h Outdated Show resolved Hide resolved
src/types/redis_json.cc Show resolved Hide resolved
src/types/redis_json.cc Show resolved Hide resolved
src/commands/cmd_json.cc Outdated Show resolved Hide resolved
src/commands/cmd_json.cc Outdated Show resolved Hide resolved
@mapleFU
Copy link
Member

mapleFU commented Oct 19, 2023

The current of CommandJsonArrAppend::Execute uses MultiBulkString. Is there a need to provide an MultiBulkInteger?

https://github.com/redis/redis-specifications/blob/master/protocol/RESP2.md#resp-integers Redis has a resp protocol, and we don't need to using "(Integer)" literal. Just using redis::Integer is ok here.

By the way, you should also add some integration test in tests/gocase. It will test the command from the client side. You can refer to unit/type/json/json_test.go

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Thanks! Just few questions

tests/cppunit/types/json_test.cc Show resolved Hide resolved
src/commands/cmd_json.cc Outdated Show resolved Hide resolved
src/types/redis_json.cc Outdated Show resolved Hide resolved
mapleFU
mapleFU previously approved these changes Oct 19, 2023
@mapleFU mapleFU requested a review from PragmaTwice October 19, 2023 16:03
@mapleFU
Copy link
Member

mapleFU commented Oct 19, 2023

Thanks @lieck . The patch lgtm!

But you still need to fix the golang-lint:

unit/type/json/json_test.go:60: File is not `gofmt`-ed with `-s` (gofmt)
	    require.NoError(t, rdb.Do(ctx, "SET", "a", `1`).Err())
        require.Error(t, rdb.Do(ctx, "JSON.ARRAPPEND", "a", "$", `1`).Err())
        require.NoError(t, rdb.Do(ctx, "DEL", "a").Err())
$ /home/runner/go/bin/golangci-lint run -v ./...
Traceback (most recent call last):

You can just run go fmt to format the file.

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Thanks @lieck . I'll wait for other committers comment, and will merge it if no other doesn't have comments on this and ci passes.

@mapleFU
Copy link
Member

mapleFU commented Oct 20, 2023

cc @git-hulk @PragmaTwice

@mapleFU mapleFU merged commit 411ebfe into apache:unstable Oct 20, 2023
28 checks passed
@mapleFU
Copy link
Member

mapleFU commented Oct 20, 2023

Thanks @lieck . Merged now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for the JSON.ARRAPPEND command
3 participants