Skip to content

Conversation

@adnan-alhomssi
Copy link

No description provided.

Copy link
Member

@andrebsguedes andrebsguedes left a comment

Choose a reason for hiding this comment

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

Looks great, some minor comments

src/crud_ops.rs Outdated
unsafe impl Send for BulkResponse {}

impl RawResponse for BulkResponse {
type Payload = Vec<(Path, String)>;
Copy link
Member

Choose a reason for hiding this comment

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

As discussed elsewhere we will fix the ergonomics of this later, but one thing we can do now is defer converting the error to string to set_payload. That way a Rust user of bulk_delete still has an Error to work with.

Suggested change
type Payload = Vec<(Path, String)>;
type Payload = Vec<(Path, crate::Error)>;

Copy link
Member

@andrebsguedes andrebsguedes left a comment

Choose a reason for hiding this comment

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

The last bunch of nits (sorry for that!)

Edit: The order of the comments got jumbled, I suggest going in the order they appear in the file

@adnan-alhomssi adnan-alhomssi marked this pull request as ready for review February 11, 2025 09:56
@adnan-alhomssi adnan-alhomssi merged commit 6580c33 into main Feb 11, 2025
4 checks 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