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

#1013 Migrating: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL') #1149

Merged
merged 1 commit into from
Nov 2, 2024
Merged

#1013 Migrating: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL') #1149

merged 1 commit into from
Nov 2, 2024

Conversation

SyedMa3
Copy link
Contributor

@SyedMa3 SyedMa3 commented Oct 18, 2024 •
edited
Loading

Migrated the following commands: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL')

Checklist

  • 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.
  • 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.

Closes: #1013

@AshwinKul28 AshwinKul28 added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 19, 2024
JyotinderSingh
JyotinderSingh requested changes Oct 19, 2024
View reviewed changes
integration_tests/commands/http/copy_test.go Outdated Show resolved Hide resolved
integration_tests/commands/websocket/set_test.go Outdated Show resolved Hide resolved
integration_tests/commands/websocket/set_test.go Outdated Show resolved Hide resolved
lucifercr07
lucifercr07 reviewed Oct 22, 2024
View reviewed changes
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.

@SyedMa3 can we validate the docs related to these command once? Meanwhile I'll confirm the HTTP behaviour it shouldn't be the way it's currently, will fix if required.

@SyedMa3
Copy link
Contributor Author

SyedMa3 commented Oct 26, 2024

@SyedMa3 can we validate the docs related to these command once? Meanwhile I'll confirm the HTTP behaviour it shouldn't be the way it's currently, will fix if required.

Since we are just migrating the commands, I did not find any changes that needed to be done in the docs.
Any update on the HTTP behavior? @lucifercr07

@lucifercr07
Copy link
Contributor

@SyedMa3 please resolve conflicts.
Have raised fix as part of #1204

@AshwinKul28
Copy link
Contributor

Hi @SyedMa3 Thanks for all the changes, looks fantastic.
Related to the document changes, I see EXPIRETIME, TTL, and PTTL commands need a refresh to get aligned with this sample documentation. I know it's a little back and forth but we want to make it production-ready. So please look at the documentation files for these commands and make it align.

@SyedMa3
Copy link
Contributor Author

SyedMa3 commented Oct 27, 2024

Hi @SyedMa3 Thanks for all the changes, looks fantastic. Related to the document changes, I see EXPIRETIME, TTL, and PTTL commands need a refresh to get aligned with this sample documentation. I know it's a little back and forth but we want to make it production-ready. So please look at the documentation files for these commands and make it align.

Oh my bad. Missed that. Will push those changes along with modified test cases after #1204 is closed.

@AshwinKul28
Copy link
Contributor

Hey hi @SyedMa3 thanks for the effort related to the refactoring. I see #1204 is now closed! you can go ahead and rebase your code with the master and have a look on refreshing the docs.

@SyedMa3 SyedMa3 force-pushed the mirate_exp branch 2 times, most recently from f290cca to 77779ae Compare October 30, 2024 19:38
@SyedMa3 SyedMa3 requested a review from lucifercr07 October 31, 2024 07:25
AshwinKul28
AshwinKul28 approved these changes Nov 2, 2024
View reviewed changes
Copy link
Contributor

@AshwinKul28 AshwinKul28 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the commendable efforts @SyedMa3

@AshwinKul28 AshwinKul28 merged commit 91277db into DiceDB:master Nov 2, 2024
2 checks passed
@SyedMa3 SyedMa3 deleted the mirate_exp branch November 2, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoorvyadav1111 apoorvyadav1111 apoorvyadav1111 left review comments

@AshwinKul28 AshwinKul28 AshwinKul28 approved these changes

@JyotinderSingh JyotinderSingh Awaiting requested review from JyotinderSingh

@lucifercr07 lucifercr07 Awaiting requested review from lucifercr07

Assignees
No one assigned
Labels
migration -- command Migrates current eval to a refactored eval for all protocols functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Command Migration: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL')
5 participants
@SyedMa3 @lucifercr07 @AshwinKul28 @apoorvyadav1111 @JyotinderSingh