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

Fixed duplicate timestamps causing pagination test failure #157

Merged
merged 3 commits into from
Sep 21, 2024

Conversation

ahmed-hossam28
Copy link
Contributor

No description provided.

@ahmed-hossam28 ahmed-hossam28 changed the title Fixed duplicate timestamps causing pagination test failure #156 Fixed duplicate timestamps causing pagination test failure Sep 20, 2024
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated
// Old code that caused duplicates:
// doc.Set("timestamp", time.Now())

// New code to ensure unique timestamps:
Copy link
Owner

@ostafen ostafen Sep 20, 2024

Choose a reason for hiding this comment

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

This comment makes sense, but there is no need to mention about what "old code" was doing

Copy link
Owner

Choose a reason for hiding this comment

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

You can leave this one, but rephrase it to not mention "old code"

db_test.go Outdated Show resolved Hide resolved
Copy link
Owner

@ostafen ostafen 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 the PR, @ahmed-hossam28. Added some comments.

@ahmed-hossam28
Copy link
Contributor Author

Thank you @ostafen for your efforts too, I will resolve them.

@ahmed-hossam28
Copy link
Contributor Author

@ostafen, I have removed the unnecessary comments and the checkUniqueTimestamps function as suggested.

db_test.go Outdated
@@ -1507,7 +1507,11 @@ func TestPagedQueryUsingIndex(t *testing.T) {
n := 10003
for i := 0; i < n; i++ {
doc := d.NewDocument()
doc.Set("timestamp", time.Now())

// doc.Set("timestamp", time.Now())
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove all the comments

Copy link
Owner

@ostafen ostafen left a comment

Choose a reason for hiding this comment

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

Added some more comments

@ahmed-hossam28
Copy link
Contributor Author

Thank you @ostafen for your feedback, i updated the comments.

@ostafen
Copy link
Owner

ostafen commented Sep 21, 2024

Looks good now, thanks for your patience!

@ostafen ostafen merged commit 2b3279f into ostafen:v2 Sep 21, 2024
1 check 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