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

[BUG][swift6] Generator does not honor ProjectName for APIs client #20056

Closed
4 of 5 tasks
lilidotshi opened this issue Nov 7, 2024 · 10 comments · Fixed by #20103
Closed
4 of 5 tasks

[BUG][swift6] Generator does not honor ProjectName for APIs client #20056

lilidotshi opened this issue Nov 7, 2024 · 10 comments · Fixed by #20103

Comments

@lilidotshi
Copy link
Contributor

lilidotshi commented Nov 7, 2024

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • [] What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

swift5 used to apply the ProjectName to the API. I see that swift6 did the same until #19732 where {{projectName}}API was changed to OpenAPIClient.

Before After
Screenshot 2024-11-07 at 3 20 31 PM Screenshot 2024-11-07 at 3 20 00 PM
openapi-generator version

v7.9.0

Generation Details

Can just use the default PetStore generation details to see the bug

Steps to reproduce

Supply projectName to the swift6 generator and observe that projectName does not get appended to the OpenAPIClient object

Related issues/PRs

#19732

Suggest a fix

Not sure if this necessarily a bug, however, we used the ProjectName to differentiate between our various OpenAPI clients and reduce collisions (without the need to name space).

@4brunu
Copy link
Contributor

4brunu commented Nov 13, 2024

Hi, this decision was made because other generators, for example kotlin client has a similar approach by having an ApiClient.
The advantage is that the central class that has all the configuration is more easily discoverable.
The disadvantage is the possible collisions when using multiple OpenAPI clients, although it's possible to use the module name to disambiguate.
I'm open to revert this, the generator is still in beta, so it's ok to introduce breaking changes, and this will make it easier for people to migrate to swift 6.
The main issue in reverting this is the users that already migrated to swift 6, but we could introduce a warning deprecating OpenAPIClient.
What do you think of this, the advantages and disadvantages?
I would also like to ask @x-sheep and @Jonas1893 for their opinion, since they already contributed to swift 6 generator.

@x-sheep
Copy link
Contributor

x-sheep commented Nov 13, 2024

I had a project using 2 outputs from the Open API Generator, since I needed to connect to two servers in the same app, and I did run into the issue that the OpenAPIClient name was used twice in different dependencies.

By default there's a module called OpenAPIClient and a type called OpenAPIClient, and Swift does not understand what you're talking about if you try to type OpenAPIClient.OpenAPIClient. I ended up adding typealiases to the project to keep them separate, but I had to put each typealias in a different source file because importing both projects in 1 source file leads to ambiguities I could not resolve.

@4brunu
Copy link
Contributor

4brunu commented Nov 13, 2024

Since this is creating issues to more than one person, I think we should revert it now, while the generator is still in beta.
I will try to create a PR with it tomorrow.

@lilidotshi
Copy link
Contributor Author

The issue @x-sheep brings up is interesting because of the default naming. I am not against the default and easy locating of having a file named OpenAPIClient, but the collision between Module and Type is a bigger problem. What about renaming it to have a default module name of OpenAPIClient and a type APIClient? Namespacing APIClient is not that big of an inconvenience, but i have no strong feelings besides having differently named modules and type.

@4brunu
Copy link
Contributor

4brunu commented Nov 13, 2024

I was thinking of naming it {{projectName}}APIClient to avoid collisions, because the old name {{projectName}}API was a bit vage.

@lilidotshi
Copy link
Contributor Author

I'm in favor of that, I didn't like the {{projectName}}API.

@4brunu
Copy link
Contributor

4brunu commented Nov 13, 2024

There is a potencial issue with {{projectName}}APIClient, for example with the PetstoreClient, it will be PetstoreClientAPIClient.
What about PetstoreClientAPIConfiguration?

@4brunu
Copy link
Contributor

4brunu commented Nov 13, 2024

It comes to mind those 3 options:
{{projectName}}API (the old one, I think it a bit ambiguous because all the api calls also end in API, but on the other hand it's the old one, so the migration from Swift 5 to Swift 6 would be somother)
{{projectName}}APIClient (I like this one, but it can introduce issues if the project name includes Client, for example PetstoreClientAPIClient)
{{projectName}}APIConfiguration (maybe it's a good option?)

@lilidotshi
Copy link
Contributor Author

So I'm not opposed to having a static name for it. I think if we did APIClient for this class, but kept OpenAPIClient as the default folder name, it would solve the problem x-sheep was seeing with the Type and Module name conflicting, and would keep it consistent with the other generators it sounds like.

@4brunu
Copy link
Contributor

4brunu commented Nov 14, 2024

I have opened #20103

I ended up using {{projectName}}APIConfiguration.

What do you all think of this?
Do you think it's a good choice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants