Skip to content

feat!: return unit type instead of wkt::Empty#1635

Merged
coryan merged 5 commits intogoogleapis:mainfrom
coryan:feat-bang-map-empty-to-unit-for-responses
Mar 27, 2025
Merged

feat!: return unit type instead of wkt::Empty#1635
coryan merged 5 commits intogoogleapis:mainfrom
coryan:feat-bang-map-empty-to-unit-for-responses

Conversation

@coryan
Copy link
Collaborator

@coryan coryan commented Mar 27, 2025

This is the normal mapping for wkt::Empty (or google.protobuf.Empty)
when used as a return type for a unary RPC.

Fixes #1509

coryan added 2 commits March 27, 2025 13:18
This is the normal mapping for `wkt::Empty` (or `google.protobuf.Empty`)
when used as a return type for a unary RPC.
@codecov
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.72%. Comparing base (345cd34) to head (d3c8122).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1635   +/-   ##
=======================================
  Coverage   95.72%   95.72%           
=======================================
  Files          43       43           
  Lines        1825     1825           
=======================================
  Hits         1747     1747           
  Misses         78       78           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coryan coryan marked this pull request as ready for review March 27, 2025 17:56
@coryan coryan requested review from codyoss and dbolduc March 27, 2025 17:56
Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

Approved, but consider my suggestions :)

Comment on lines +86 to +102
{{#ReturnsEmpty}}
let _ : wkt::Empty = self.inner.execute(
builder,
{{#PathInfo.Codec.HasBody}}Some(req{{Codec.BodyAccessor}}){{/PathInfo.Codec.HasBody}}
{{^PathInfo.Codec.HasBody}}None::<gaxi::http::NoBody>{{/PathInfo.Codec.HasBody}},
options,
).await?;
Ok(())
{{/ReturnsEmpty}}
{{^ReturnsEmpty}}
self.inner.execute(
builder,
{{#PathInfo.Codec.HasBody}}Some(req{{Codec.BodyAccessor}}){{/PathInfo.Codec.HasBody}}
{{^PathInfo.Codec.HasBody}}None::<gaxi::http::NoBody>{{/PathInfo.Codec.HasBody}},
options,
).await
{{/ReturnsEmpty}}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe too cute, but consider:

        self.inner.execute(
            builder,
            {{#PathInfo.Codec.HasBody}}Some(req{{Codec.BodyAccessor}}){{/PathInfo.Codec.HasBody}}
            {{^PathInfo.Codec.HasBody}}None::<gaxi::http::NoBody>{{/PathInfo.Codec.HasBody}},
            options,
        ).await
        {{#ReturnsEmpty}}
        .map(|_| ())
        {{/ReturnsEmpty}}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... because the execute function is generic, we have to say .map(|_: wkt::Empty| ()) It is a bit weird. If I was typing the code that is not how I would do it, but it does express what we are doing.

traits (e.g. `std::clone::Clone`) making the call ambiguous.
Using `*self.0.stub` makes the call not-ambiguous.
}}
{{#ReturnsEmpty}}
Copy link
Member

Choose a reason for hiding this comment

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

optional: we could avoid all of the ReturnsEmpty blocks (except for the one in transport.rs) if we add a ReturnType to the message1 codec, that is QualifiedName == "wkt::Empty" ? "()" : QualifiedName

up to you if you think that would be nicer.

Footnotes

  1. It probably makes more sense on the method codec, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@coryan coryan enabled auto-merge (squash) March 27, 2025 20:06
@coryan coryan merged commit 4dad35d into googleapis:main Mar 27, 2025
20 checks passed
@coryan coryan deleted the feat-bang-map-empty-to-unit-for-responses branch March 27, 2025 20:22
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.

Replace google.protobuf.Empty with the unit type as returns from RPCs

2 participants