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 Status with Details Constructor #308

Merged

Conversation

SkamDart
Copy link
Contributor

Changes are just a Copy pasta of this pr in tower-rs.

Motivation

Reference #292 for motivation.

Solution

The prescribed solution exposes two constructors with Status::with_details and Status::with_raw_details that allow the caller to construct a Status with a Code, String, and Bytes or a Message to be converted into bytes.

@SkamDart
Copy link
Contributor Author

SkamDart commented Mar 29, 2020

Where should I add the tests? Wasn't quite sure where to place them as they require a generated proto fixture.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Looks like a great start, left one comment once that is addressed I think we can merge!

}

/// Create a new `Status` with the associated code, message, and protobuf details field.
pub fn with_details(code: Code, message: impl Into<String>, details: impl Message) -> Status {
Copy link
Member

Choose a reason for hiding this comment

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

I actually believe we should not include a prost based one here. I think we can just have the one above that takes a Bytes and we remove the impl Into. We can also rename the one above to with_details. then let the user choose if they want to encode via prost or something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Just copy pasta from the PR I inherited. I'm fairly new to Rust so I appreciate your patience. Is this cargo check --no-default-features CI check to enforce that there's no unnecessary coupling of external crates to the core library?

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Thanks!

@LucioFranco LucioFranco changed the title Add Status with Details Constructor feat: Add Status with Details Constructor Mar 30, 2020
@LucioFranco LucioFranco merged commit cfd59db into hyperium:master Mar 30, 2020
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