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

Additional api protocol conformances #635

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

peterringset
Copy link

Motivation

The generated code from this tool offers a very good starting point for integrating an API into a larger project. One challenge that I've run into when using this tool is testability and mocking. While it is possible to write mocks by hand I have found it very useful to use code generation tools (e.g. Sourcery) to do most of the heavy lifting. To enable behavior like this it is often necessary to annotate the client/server source in some way. This change offers a way to let the APIProtocol declaration have a conformance to one or more user-defined protocols to achieve this.

Modifications

  • Added an option to the generate command to list protocols that the user wishes the API to conform to
  • Added entry to the config-file to list protocols that the user wishes the API to conform to
  • Added protocol conformance of user-defined protocols to the APIProtocol
  • Added test to verify correct behavior

Result

The generate command and config file will get an extra option (additionalAPIProtocols) for the user to list additional protocols they wish that the APIProtocol type conforms to.

Test Plan

  • Added a reference test that generates an empty API with conformance to an additional example protocol
  • Modified existing config file test to include this change

@simonjbeaumont
Copy link
Collaborator

@peterringset thanks for putting up this patch and your interest in the project!

We've heard of folks wanting this before (e.g. #573, #383).

While I'm not against it, I'd like to understand the motivation more. Unlike the issue that asked to attach user-defined macros, IIUC this PR would allow users only to declare a protocol conformance on the generated protocol APIProtocol. Some questions:

What's the developer experience when the protocol doesn't exist? Presumably the generated code just fails to compile but we need to make sure this surfaces well, because that conformance is now in a derived source file.

What's next for the adopter? Are we relying on them having to go and provide the protocol witnesses to actually conform to the protocol? If this is the case then does this bring much value? It seems like it will just separate the conformance from the conformance declaration in user code, which might actually be more confusing?

Potentially this is mainly focused on protocols that rely only on default implementations so declaring conformance is enough?

@peterringset
Copy link
Author

Hi! Thanks for the quick feedback. I'll write my replies one by one below:

What's the developer experience when the protocol doesn't exist? Presumably the generated code just fails to compile but we need to make sure this surfaces well, because that conformance is now in a derived source file.

Yes, that's true. My initial thought would be that this is something that the developer opts in to, and it's hopefully not something that they would unintentionally add to their code generation. With that in mind I hope that all developers adopting this will understand how to use this option and declare the necessary protocol(s) in a different file that's available in the generated code's scope. I don't know if there's anything more that could/should be done? Maybe adding some comments to document what's happening?

What's next for the adopter? Are we relying on them having to go and provide the protocol witnesses to actually conform to the protocol? If this is the case then does this bring much value? It seems like it will just separate the conformance from the conformance declaration in user code, which might actually be more confusing?

This usage would in most cases mean that the protocol(s) to be added already exists somehow in the package that we're generating code in. In my particular case it's necessary for APIProtocol to inherit from the custom protocols (AutoMockable in my case with Sourcery) for the mocks generation to work properly. I.e. protocol APIProtocol: Sendable, AutoMockable { ... works, but extension APIProtocol: AutoMockable { ... does not work, since protocols cannot declare inheritance to other protocols like that. This allows me to generate a mock for APIProtocol, and not for Client.

Another use case could be that the custom protocol requirements exactly match the APIProtocols signatures, such that APIProtocol is already implementing the protocol by just declaring conformance. Although, this could also be solved other ways.

Potentially this is mainly focused on protocols that rely only on default implementations so declaring conformance is enough?

Yep, I think that is the most common use-case for a feature like this. The Sourcery use-case for instance has an empty protocol AutoMockable that works kind of like an annotation, and allows Sourcery to perform the mock generation for protocols that inherit from AutoMockable.

@czechboy0
Copy link
Contributor

Hi @peterringset,

yeah this is an interesting situation. I'm also not necessarily against landing this PR, but I'd like to explore how much this really helps compared to:

public typealias AutoMockableAPIProtocol = APIProtocol & AutoMockable

and then using AutoMockableAPIProtocol in your codebase, which has the advantage of not mixing the generated code from the two separate tools? Or would Sourcery not generate the code if the conformance is declared in the typealias?

@peterringset
Copy link
Author

@czechboy0 No, unfortunately Sourcery doesn't pick up the AutoMockableAPIProtocol typealias as an eligible type for mocks. AFAIK it only cares about protocols directly inheriting AutoMockable, so composition will also not work in this particular case.

@czechboy0
Copy link
Contributor

I see. From briefly skimming its sources, I wonder if support for inspecting typealiases could be added, something around https://github.com/krzysztofzablocki/Sourcery/blob/8acc3bf2ba1828e5f1126c1acc23a9e79561652d/Templates/Templates/AutoMockable.stencil#L335C10-L335C55 ?

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Marking as blocked for now, we suggested a possible workaround above. If that doesn't work, we can discuss continuing to move this PR forward, however.

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