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 command GEOPOS #1328

Merged
merged 18 commits into from
Dec 10, 2024
Merged

Add support for command GEOPOS #1328

merged 18 commits into from
Dec 10, 2024

Conversation

helios2003
Copy link
Contributor

@helios2003 helios2003 commented Nov 25, 2024

The PR aims to resolve issue #1249

Tasks

  • Add the support for the command.
  • Add the docs.
  • Add integration tests.
  • Add unit tests.

@helios2003 helios2003 marked this pull request as draft November 25, 2024 14:07
@helios2003
Copy link
Contributor Author

Hi @lucifercr07, I have made a draft PR for the issue. I will update it with all the changes in the next few days.

@helios2003 helios2003 marked this pull request as ready for review December 1, 2024 08:54
@helios2003 helios2003 closed this Dec 1, 2024
@helios2003 helios2003 reopened this Dec 1, 2024
@helios2003
Copy link
Contributor Author

@lucifercr07, can you please review the PR? After @arpitbbhayani's recent pushes, the tests are failing now. Can you please give me some pointers on how to adapt the code to the latest master branch?

@lucifercr07
Copy link
Contributor

@lucifercr07, can you please review the PR? After @arpitbbhayani's recent pushes, the tests are failing now. Can you please give me some pointers on how to adapt the code to the latest master branch?

@helios2003 will check once. Thanks.

@helios2003
Copy link
Contributor Author

@lucifercr07, any updates on the review process?

Copy link
Contributor

@lucifercr07 lucifercr07 left a comment

Choose a reason for hiding this comment

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

LGTM

expected: []interface{}{[]interface{}{nil}},
},
{
name: "GEOPOS for non-existing index",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add a test for below case?

redis> SET k1 v1
"OK"
redis> GEOPOS k1 v1
(error) WRONGTYPE Operation against a key holding the wrong kind of value

@helios2003
Copy link
Contributor Author

@lucifercr07, I have made the required changes.

@lucifercr07 lucifercr07 merged commit df110f6 into DiceDB:master Dec 10, 2024
2 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.

2 participants