-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Swift5 Vapor 4 client library #9583
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e994d4e
[swift5] Add useClasses cliOption
aymanbagabas fbe571c
[swift5] Add Vapor client library
aymanbagabas 549cba9
[swift5] Fix AnyCodable missing in api.mustache and conform it to
aymanbagabas e80c26f
[swift5] Fix encoding body using ContentConfiguration
aymanbagabas c76132c
Update samples and docs
aymanbagabas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
generatorName: swift5 | ||
outputDir: samples/client/petstore/swift5/vaporLibrary | ||
library: vapor | ||
inputSpec: modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml | ||
templateDir: modules/openapi-generator/src/main/resources/swift5 | ||
generateAliasAsModel: true | ||
additionalProperties: | ||
projectName: PetstoreClient | ||
useSPMFileStructure: true | ||
useClasses: true | ||
useBacktickEscapes: true | ||
mapFileBinaryToData: true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aymanbagabas this is a breaking change that affects a lot of file, and what I remember from the other PR is that this change was only related to naming, so I suggest to revert it please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's only related to naming. However, it doesn't make sense to forcefully append "API" to the project name. The user should have a choice of having that in the name. Simply when desired, the user can append "API" to the project name and it would give the old behavior back.
I'll make a separate PR for that one. Should I base it on top of the 5.2.x branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't think this is an option worth to create.
It will add extra complexity and we won't add nothing new or extra functionality to the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aesthetics matter, having this suffix in the name is extra complexity. By merging it to the 5.2.x branch, we will introduce a breaking change with fallback and we don't have to introduce any flags or options since the fallback is to have
API
in the project name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that would break every project that uses OpenAPI Swift Generator with compilation errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but for a generated library, I would rather have the project name matches the library class name since there is only one class. This also matches the SPM file structure for example the
PlayingCard
libraryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point.
My main issue here is creating an extra burden on the user side.
To try to make this transition as smooth as possible, we could do something like this.
This way the old name PlayingCardAPI would still work, and the user could migrate when he wanted it.
Plus the IDE would presente a warning in a quick fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a yes? Should I create a PR for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You last comment made sense to me, so for me it's a yes.
Could you please open a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@4brunu I'm also thinking about doing a similar thing for
Configuration.mustache
andmodel.mustache
when using theswiftUseApiNamespace
option. I believe when users enableswiftUseApiNamespace
, the Configuration class and models should also be under the API namespace like whatapi.mustache
has. Because it doesn't make sense to do so for the API classes and not for the other classes. For example:and