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(build): Add #[deprecated] to deprecated client methods #1879

Merged
merged 3 commits into from
Aug 26, 2024
Merged

feat(build): Add #[deprecated] to deprecated client methods #1879

merged 3 commits into from
Aug 26, 2024

Conversation

bytedream
Copy link
Contributor

@bytedream bytedream commented Aug 20, 2024

Motivation

When an rpc method is marked as deprecated, tonic currently doesn't forward it to the user (#1512).

Solution

This PR adds the #[deprecated] attribute to generated rust client methods that are marked as deprecated in a protobuf file. I've added Method::deprecated which returns if the method is deprecated and is checked when the client code is generated. I'd consider this a breaking change, as the Method trait is publicly exposed and the code will now emit warnings if a deprecated generated method is called. Method::deprecated has a default implementation which always returns false.

@@ -144,6 +144,8 @@ pub trait Method {
fn server_streaming(&self) -> bool;
/// Get comments about this item.
fn comment(&self) -> &[Self::Comment];
/// Method is deprecated.
fn deprecated(&self) -> bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we give this a default implementation that yields false? That would make this a non-breaking change, which might be nice, and it seems conceptually reasonable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now I agree to make this setting false by default, and I think it would be good to set it to true in the future breaking release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, I've added it

Copy link
Contributor

Choose a reason for hiding this comment

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

For now I agree to make this setting false by default, and I think it would be good to set it to true in the future breaking release.

Why? That doesn't make much sense to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I intended to mean was to not mark the code generated from the proto's deprecated as deprecated by default this time, and to enable it in the next major release. And I just noticed that I was commenting on something different than the perspective of your review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so you mean removing the default implementation when we release 0.13? That makes sense to me.

@djc djc added this pull request to the merge queue Aug 26, 2024
Merged via the queue into hyperium:master with commit 1c5150a Aug 26, 2024
16 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.

3 participants