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 support for JSON.STRAPPEND and JSON.STRLEN command #1841

Merged
merged 18 commits into from
Dec 5, 2023

Conversation

guojidan
Copy link
Contributor

@guojidan guojidan commented Oct 20, 2023

This closes #1823 #1824

src/commands/cmd_json.cc Outdated Show resolved Hide resolved
src/commands/cmd_json.cc Outdated Show resolved Hide resolved
src/commands/cmd_json.cc Outdated Show resolved Hide resolved
tests/gocase/unit/type/json/json_test.go Outdated Show resolved Hide resolved
src/types/redis_json.cc Show resolved Hide resolved
@PragmaTwice PragmaTwice changed the title support JSON.STRAPPEND Add support for command JSON.STRAPPEND Oct 20, 2023
@guojidan
Copy link
Contributor Author

I fixed the problem you pointed out : )

src/types/json.h Outdated Show resolved Hide resolved
src/types/redis_json.cc Outdated Show resolved Hide resolved
src/commands/cmd_json.cc Outdated Show resolved Hide resolved
@guojidan guojidan changed the title Add support for command JSON.STRAPPEND Add support for command JSON.STRAPPEND && JSON.STRLEN Oct 20, 2023
src/types/json.h Outdated Show resolved Hide resolved
src/types/json.h Outdated Show resolved Hide resolved
src/types/json.h Outdated Show resolved Hide resolved
src/types/json.h Outdated Show resolved Hide resolved
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

Even if you want to use -1 as a flag number, you DO NOT need to change unsigned integer type to signed integer. It is useless and make the situation more complicted.

If you don't get this point, you can refer to https://en.cppreference.com/w/cpp/string/basic_string/npos.

It also gonna be a blocking issue of this PR.

@guojidan
Copy link
Contributor Author

Even if you want to use -1 as a flag number, you DO NOT need to change unsigned integer type to signed integer. It is useless and make the situation more complicted.

If you don't get this point, you can refer to https://en.cppreference.com/w/cpp/string/basic_string/npos.

It also gonna be a blocking issue of this PR.

I understand

@guojidan guojidan requested a review from PragmaTwice October 27, 2023 02:43
@PragmaTwice
Copy link
Member

Hi @guojidan, before we review this PR, you can first try to solve these conflicts with unstable.

@guojidan
Copy link
Contributor Author

Hi @guojidan, before we review this PR, you can first try to solve these conflicts with unstable.

hi @PragmaTwice Thank you very much for your help, now I already rebase this pr

src/types/json.h Outdated Show resolved Hide resolved
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

Sorry but it still has several issues which need to be addressed before merging.

@guojidan guojidan requested a review from PragmaTwice November 1, 2023 06:25
src/types/redis_json.cc Outdated Show resolved Hide resolved
src/types/redis_json.cc Outdated Show resolved Hide resolved
src/types/json.h Outdated Show resolved Hide resolved
src/types/json.h Outdated Show resolved Hide resolved
@guojidan
Copy link
Contributor Author

guojidan commented Dec 4, 2023

@guojidan Are you still working on this PR?

@guojidan Are you still working on this PR?

yes, this pr wait review

@guojidan guojidan requested a review from git-hulk December 4, 2023 02:59
git-hulk
git-hulk previously approved these changes Dec 4, 2023
@git-hulk
Copy link
Member

git-hulk commented Dec 4, 2023

Some places need to be improved, but I will do it in another PR:

  • Keep consistent of the integer receiver, should be std::vector<std::optional<uint64_t>>.
  • Add a function to covert std::vector<std::optional<uint64_t>> to the Redis output to avoid duplicate code blocks.

To avoid PR being pending too long and those improvements are code style-related. So it's good for me to approve this PR first and improve it later.

@git-hulk
Copy link
Member

git-hulk commented Dec 5, 2023

@guojidan Cloud you please resolve conflicts again

@git-hulk git-hulk changed the title Add support for command JSON.STRAPPEND && JSON.STRLEN Add support for JSON.STRAPPEND and JSON.STRLEN command Dec 5, 2023
Copy link

sonarcloud bot commented Dec 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
5.4% 5.4% Duplication

@git-hulk git-hulk merged commit bf69410 into apache:unstable Dec 5, 2023
30 checks passed
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.STRAPPEND command
4 participants