Add deprecation annotations for RPCs marked "deprecated=true" #316

eseay · 2024-10-24T15:33:38Z

This enhances the ConnectClientGenerator to inspect the deprecated option and annotate the generated service functions accordingly.

For async/await functions, the existing @available annotation, which only exists for iOS, is adjusted accordingly to introduce and deprecate the function at v13, since that is when concurrency was first made available.
- @rebello95 should we consider adjusting both my deprecation annotation and the existing annotation to account for the other supported platforms of the library?
Since no pattern was established for generated callback functions, I enhanced their generation to include @available annotations for all supported platforms. The "introduced" and "deprecated" versions for those platforms were determined by the minimum supported version of the respective platform defined in Package.swift.

Adding the following RPC...

rpc SayDeprecated(SayRequest) returns (SayResponse) {
  option idempotency_level = NO_SIDE_EFFECTS;
  option deprecated = true;
}

...to the ElizaService defined in the examples-go repository, from which the Eliza sample app sources are generated, and regenerating the Swift service (setting GenerateCallbackMethods=true to also demonstrate callback behavior) yields the following result:

@available(iOS, introduced: 12, deprecated: 12, message: "This function has been marked deprecated.")
@available(macOS, introduced: 10.15, deprecated: 10.15, message: "This function has been marked deprecated.")
@available(tvOS, introduced: 13, deprecated: 13, message: "This function has been marked deprecated.")
@available(watchOS, introduced: 6, deprecated: 6, message: "This function has been marked deprecated.")
@discardableResult
func `sayDeprecated`(request: Connectrpc_Eliza_V1_SayRequest, headers: Connect.Headers, completion: @escaping @Sendable (ResponseMessage<Connectrpc_Eliza_V1_SayResponse>) -> Void) -> Connect.Cancelable

@available(iOS, introduced: 13, deprecated: 13, message: "This function has been marked deprecated.")
func `sayDeprecated`(request: Connectrpc_Eliza_V1_SayRequest, headers: Connect.Headers) async -> ResponseMessage<Connectrpc_Eliza_V1_SayResponse>

The generated code for the existing Say RPC remains unchanged:

/// Say is a unary RPC. Eliza responds to the prompt with a single sentence.
@discardableResult
func `say`(request: Connectrpc_Eliza_V1_SayRequest, headers: Connect.Headers, completion: @escaping @Sendable (ResponseMessage<Connectrpc_Eliza_V1_SayResponse>) -> Void) -> Connect.Cancelable

/// Say is a unary RPC. Eliza responds to the prompt with a single sentence.
@available(iOS 13, *)
func `say`(request: Connectrpc_Eliza_V1_SayRequest, headers: Connect.Headers) async -> ResponseMessage<Connectrpc_Eliza_V1_SayResponse>

The same additions have been made to the ConnectMockGenerator.

Closes #305

Signed-off-by: Eddie Seay <[email protected]>

eseay

A couple in-context notes for y'all to look at as well @rebello95 @jhump. Thanks!

eseay · 2024-10-24T18:02:13Z

Plugins/ConnectMocksPlugin/ConnectMockGenerator.swift

+
+    func callbackAvailabilityAnnotation() -> String? {
+        if self.options.deprecated {
+            // swiftlint:disable line_length


@rebello95 @jhump Let me know if this is acceptable. I use SwiftLint in my own repos as well, so I know the rules exist for a purpose; however, I may propose disabling this specific rule in this specific context to improve legibility in the editor and to more closely mimic what will actually be output at the time of generation.

This seems fine to me, doing additional gymnastics would be a bit excessive 😉

eseay · 2024-10-24T18:04:29Z

Plugins/ConnectMocksPlugin/ConnectMockGenerator.swift

+    func asyncAwaitAvailabilityAnnotation() -> String? {
+        if self.options.deprecated {
+            // swiftlint:disable:next line_length
+            return "@available(iOS, introduced: 13, deprecated: 13, message: \"This function has been marked deprecated.\")"
+        } else {
+            return nil
+        }
+    }


This function signature and behavior differs from its sibling defined in ConnectClientGenerator since the entire type is already annotated with an @available(iOS 13, *) in the generated mock. Because of that existing annotation, we only need to produce something if in the case that it's more restrictive than the availability rules defined at the higher scope.

rebello95 · 2024-10-26T04:48:13Z

@rebello95 should we consider adjusting both my deprecation annotation and the existing annotation to account for the other supported platforms of the library?

Yes, I think that would be great 👍🏽 thanks for flagging.

rebello95

Thanks for implementing this!

rebello95 · 2024-10-26T04:44:55Z

Plugins/ConnectMocksPlugin/ConnectMockGenerator.swift

+
+    func callbackAvailabilityAnnotation() -> String? {
+        if self.options.deprecated {
+            // swiftlint:disable line_length


This seems fine to me, doing additional gymnastics would be a bit excessive 😉

rebello95 · 2024-10-26T04:45:55Z

Plugins/ConnectMocksPlugin/ConnectMockGenerator.swift

+        if self.options.deprecated {
+            // swiftlint:disable line_length
+            return """
+            @available(iOS, introduced: 12, deprecated: 12, message: "This function has been marked deprecated.")


Thoughts on changing these messages to "This RPC has been marked as deprecated in its .proto file."?

Even clearer - a good move in my book!

rebello95 · 2024-10-26T04:49:37Z

Plugins/ConnectSwiftPlugin/ConnectClientGenerator.swift

@@ -201,6 +207,30 @@ private extension MethodDescriptor {
        }
    }

+    func callbackAvailabilityAnnotation() -> String? {


I think all these helper functions can be private

I left them without explicit access to align with the functions that were already there; however, I think we're good on not needing to specify access level since the enclosing extension private extension MethodDescriptor is already marked.

Ah didn't realize these were inside a private extension 👍🏽

Signed-off-by: Eddie Seay <[email protected]>

eseay requested a review from rebello95 October 24, 2024 15:34

305: Acknowledge deprecated option

4ee7dcb

Signed-off-by: Eddie Seay <[email protected]>

eseay force-pushed the eseay/305-deprecation branch from 6be74d8 to 4ee7dcb Compare October 24, 2024 17:36

305: Apply deprecation annotations for generated mocks

24ae23d

Signed-off-by: Eddie Seay <[email protected]>

eseay commented Oct 24, 2024

View reviewed changes

eseay requested a review from jhump October 24, 2024 18:05

eseay marked this pull request as ready for review October 24, 2024 18:05

rebello95 approved these changes Oct 26, 2024

View reviewed changes

eseay requested a review from rebello95 October 28, 2024 14:27

305: PR feedback

dd40511

Signed-off-by: Eddie Seay <[email protected]>

rebello95 approved these changes Oct 28, 2024

View reviewed changes

eseay merged commit cbfac0a into main Oct 28, 2024
15 checks passed

eseay deleted the eseay/305-deprecation branch October 28, 2024 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecation annotations for RPCs marked "deprecated=true" #316

Add deprecation annotations for RPCs marked "deprecated=true" #316

eseay commented Oct 24, 2024 •

edited

Loading

eseay left a comment

eseay Oct 24, 2024

rebello95 Oct 26, 2024

eseay Oct 24, 2024

rebello95 commented Oct 26, 2024

rebello95 left a comment

rebello95 Oct 26, 2024

rebello95 Oct 26, 2024

eseay Oct 28, 2024

rebello95 Oct 26, 2024

eseay Oct 28, 2024

rebello95 Oct 28, 2024

Add deprecation annotations for RPCs marked "deprecated=true" #316

Add deprecation annotations for RPCs marked "deprecated=true" #316

Conversation

eseay commented Oct 24, 2024 • edited Loading

eseay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebello95 commented Oct 26, 2024

rebello95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseay commented Oct 24, 2024 •

edited

Loading