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

feat: add batch delete util #293

Merged
merged 10 commits into from
Apr 6, 2023
Merged

feat: add batch delete util #293

merged 10 commits into from
Apr 6, 2023

Conversation

pgautier404
Copy link
Contributor

No description provided.

@pgautier404 pgautier404 force-pushed the batch-delete-util branch 4 times, most recently from bcd6948 to 6f31553 Compare April 4, 2023 23:57
@pgautier404 pgautier404 force-pushed the batch-delete-util branch 2 times, most recently from b70fa43 to 9f70a7c Compare April 5, 2023 20:18
@pgautier404 pgautier404 marked this pull request as ready for review April 5, 2023 20:38
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

a couple of minor questions/nits but overall looks great!

Key: myKey,
})
if err != nil {
errChan <- fmt.Sprintf("error deleting key %s: %s", myKey, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

just out of curiosity - is there a reason you decided to just push strings onto this channel rather than the actual errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly so they'd provide information about which keys failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your comment got me thinking, and I switched this up to use a map from failing key to error message instead in a6353c2. It ended up being a little more complicated to consume, and if you think its overcomplicated let me know.

batchutils/batch_operations.go Outdated Show resolved Hide resolved
cprice404
cprice404 previously approved these changes Apr 6, 2023
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

one minor naming suggestion which you can take or leave

batchutils/batch_operations.go Outdated Show resolved Hide resolved
@pgautier404 pgautier404 merged commit 327415a into main Apr 6, 2023
@pgautier404 pgautier404 deleted the batch-delete-util branch April 6, 2023 18:51
@poppoerika poppoerika mentioned this pull request May 2, 2023
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