-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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 #9625
Conversation
749d850
to
c66c103
Compare
Before review this PR, let's wait for #9624 to get merged. |
a4cfd4a
to
0acee34
Compare
Hey, I opened a PR with some spacing and CI improvements here aymanbagabas#17. |
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 could you please take a look at those two build errors? Thanks
samples/client/petstore/swift5/vaporLibrary/Sources/PetstoreClient/Extensions.swift
Outdated
Show resolved
Hide resolved
samples/client/petstore/swift5/vaporLibrary/Sources/PetstoreClient/Extensions.swift
Outdated
Show resolved
Hide resolved
* [swift5] try to remove changes in model and api * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects * [swift5] update sample projects
7c639c4
to
bda7f3b
Compare
@aymanbagabas thanks for all the work until now, and for extracting a lot of PRs from the original one on vapor. One question that I would like to have your opinion on is, you choose an enum to represent the http status code, and I was thinking in suggesting to use a just one response object with an int for http status code. EDIT: After reviewing it once more, I left some questions for you. |
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.
I left some questions that I would like to discuss with you
public enum PlaceOrder { | ||
case http200(value: Order, raw: ClientResponse) | ||
case http400(value: Void, raw: ClientResponse) | ||
case http0(value: Order, raw: ClientResponse) |
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 I think at least the default case, should have an int with the status code, because it can be something that may be important for debug proposes like 401 or 403 for example.
And Maybe instead of http0
it could be httpUnknown
to be more clearer?
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.
ClientResponse
type has a .status
property that has the response code. I agree, httpUnknown
is more clearer. Also, I think that case should optionally decode the response since we don't really know what the response would look like.
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 are right, I haven't noticed that ClientResponse has http status code, headers, etc, that's nice 👍
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.
yeah, Vapor is a really cool framework, it forces you to write beautiful code. You should try it sometime. There's vapor-server-codegen, I think bringing that to the OpenAPI generator would be a great idea.
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.
When you have time, please try to change http0
to httpUnknown
and check the value
Type in each enum case, to see what makes more sense, for us to merge 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.
After running some tests, http0
is named that way because the generator returns status code 0
when it's the default case. Thus, renaming it to httpUnknown
is not possible. What I would do is, when there is no default case, .http0
would only contain a ClientResponse
instance. Otherwise, the default case defines what type should be expected in the return.
samples/client/petstore/swift5/vaporLibrary/Sources/PetstoreClient/APIs/StoreAPI.swift
Show resolved
Hide resolved
@@ -275,7 +275,7 @@ extension {{projectName}} { | |||
case http{{code}}(value: {{#dataType}}{{{dataType}}}{{/dataType}}{{^dataType}}Void{{/dataType}}, raw: ClientResponse) |
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 I think only the status code should have 200 should have something like this
If it's 400 we don't know the response type.
case http200(value: Double, raw: ClientResponse)
case http400(value: ????, raw: ClientResponse)
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.
Per the specs, you can absolutely define responses for any status code. Maybe I should remove the value and only return ClientResponse
when the response is empty instead of returning Void
for the value. What do you think?
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.
If you are able to set te right responses for each status code, I think that would be great.
But it that is too much work, returning only the ClientResponse
would also do it.
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.
That's how it is right now, when the response in the specs is empty, it returns Void
for the value. As long as the specs contain a response schema, it would have that in the value. I will change it to ClientResponse
only when the response is empty aka no dataType
for the response.
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.
I think now it put's the success response in all status code's, or am I wrong?
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.
No, the Petstore specs are different, for examples 2_0/petstore.yaml
doesn't have a 200 response schema on POST /pet
while the 3_0
version does. Thus, the generated code for 2_0
doesn't have a 200 response enum case for POST /pet
aka addPet
operation.
393172d
to
37dbfdb
Compare
CI is failing because there are some stuff that are outdated.
Thanks |
37dbfdb
to
1d62252
Compare
looks like something is causing the |
CI failure is not related to this PR. |
@aymanbagabas Thanks for your effort in contributing with the vapor integration, and also in splitting the initial big PR into smaller ones that made multiple improvements to the Swift generator 👍 |
This PR includes all commits from #9624 and #9608
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
,5.1.x
,6.0.x