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

#1033 Migrating GEOADD and GEODIST command #1185

Merged
merged 7 commits into from
Nov 17, 2024

Conversation

YuvrajGosain
Copy link
Contributor

@YuvrajGosain YuvrajGosain commented Oct 22, 2024

Issue: #1033

  • Migrated the evalXXX function with the latest definition
  • Update or add unit tests for the new implementation.
  • All unit tests pass successfully.
  • Ensure all integration tests pass successfully.
  • Ensure integration tests are added for the migrated command on multi-threaded resp server.
  • Move Integration tests for the respective commands under the RESP integration tests directory from Async directory
  • Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

@YuvrajGosain YuvrajGosain changed the title Migrating GEOADD and GEODIST command #1033 Migrating GEOADD and GEODIST command Oct 22, 2024
@AshwinKul28 AshwinKul28 added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 22, 2024
@AshwinKul28 AshwinKul28 linked an issue Oct 24, 2024 that may be closed by this pull request
4 tasks
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.

@YuvrajGosain can follow sample doc template and update for these commands accordingly it'd be helpful.

@lucifercr07
Copy link
Contributor

@YuvrajGosain please resolve conflicts once.

@AshwinKul28
Copy link
Contributor

@YuvrajGosain hope you are doing well. Can you please update latest changes on this PR?

@YuvrajGosain
Copy link
Contributor Author

Hi @AshwinKul28 , @lucifercr07 , Apologies for the late reply , was busy in some personal stuff and wasn't able to check the status for this, I'll rebase the PR and add docs for these commands by today.

@YuvrajGosain
Copy link
Contributor Author

Hi @AshwinKul28 , @lucifercr07 , I have rebased this branch with the master and added the docs for both the commands as well.

docs/src/content/docs/commands/GEOADD.md Outdated Show resolved Hide resolved
docs/src/content/docs/commands/GEODIST.md Outdated Show resolved Hide resolved
integration_tests/commands/http/geo_tests.go Outdated Show resolved Hide resolved
integration_tests/commands/resp/geo_tests.go Outdated Show resolved Hide resolved
@JyotinderSingh
Copy link
Collaborator

@YuvrajGosain Please address the remaining review comments

@YuvrajGosain
Copy link
Contributor Author

@YuvrajGosain Please address the remaining review comments

@JyotinderSingh Hi , I have addressed the comments and rebased the branch with master. Pls have a look

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 massive migration effort @YuvrajGosain! The changes mostly look good, just had a few minor comments.

docs/src/content/docs/commands/GEOADD.md Outdated Show resolved Hide resolved
docs/src/content/docs/commands/GEODIST.md Outdated Show resolved Hide resolved
docs/src/content/docs/commands/GEODIST.md Outdated Show resolved Hide resolved
@JyotinderSingh JyotinderSingh merged commit 80e16f6 into DiceDB:master Nov 17, 2024
@YuvrajGosain
Copy link
Contributor Author

Hi @JyotinderSingh , just realized that I also pushed the changes onto the branch simultaneously and saw it being merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration -- command Migrates current eval to a refactored eval for all protocols functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command Migration: ('GEOADD', 'GEODIST')
6 participants