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

Update delete_item logic to handle json partitions #1018

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

KaspariK
Copy link
Member

@KaspariK KaspariK commented Jan 8, 2025

We need to update the logic for deleting items to account for a difference in partitions between pickles and json

@KaspariK KaspariK requested a review from a team as a code owner January 8, 2025 17:26
assert_equal(num_partitions, 0)
assert_equal(num_json_val_partitions, 0)

def test_delete_item_with_json_partitions(self, store, small_object, large_object):
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, maybe it would make more sense to test with the larger object here

Copy link
Member

Choose a reason for hiding this comment

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

imo, we have both as fixtures in this test already so we could easily test with both :)

nemacysts
nemacysts previously approved these changes Jan 9, 2025
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

just left some non-blocking suggestions/questions :)

assert_equal(num_partitions, 0)
assert_equal(num_json_val_partitions, 0)

def test_delete_item_with_json_partitions(self, store, small_object, large_object):
Copy link
Member

Choose a reason for hiding this comment

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

imo, we have both as fixtures in this test already so we could easily test with both :)

@KaspariK KaspariK merged commit 875d617 into master Jan 9, 2025
4 checks passed
@KaspariK
Copy link
Member Author

KaspariK commented Jan 9, 2025

I'll add that last comment as a todo lol

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