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

[Swift4]Update templates for swift 4 #6921

Merged
merged 15 commits into from
Nov 21, 2017

Conversation

d-date
Copy link
Contributor

@d-date d-date commented Nov 9, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the PR

1. Updated Dependencies

Updated swift4 & 3 dependencies due to RxSwift 4.0.0 was released.

To build generated source code with ResponseAs=RxSwift in Swift 4, we need to build RxSwift 4.0.0 via Carthage / Cocoapods.

Also, Alamofire 4.5.1 is needed for Swift 4.

In this PR, I fixed minimum version for dependencies.

2. Update syntax for Swift 4

According SE-0110: Distinguish between single-tuple and multiple-argument function types
, we need to use tuple f(()) insted of f(),

so when function returns Observable<Void> type, we need to change onNext() to onNext(()).

3. Adoption for Codable

When using Codable, which is introduced on Swift4, with class inheritance, we must call try super.init(from: decoder) .

@wing328
Copy link
Contributor

wing328 commented Nov 9, 2017

@d-date thanks for the PR.

cc @jgavris @ehyche @Edubits @jaz-ah

@d-date
Copy link
Contributor Author

d-date commented Nov 10, 2017

@wing328 @jgavris @ehyche @Edubits @jaz-ah
Added more improvement for swift syntax. please check it out.

@d-date d-date changed the title Update templates for swift 4 [Swift4]Update templates for swift 4 Nov 10, 2017
@jgavris
Copy link
Contributor

jgavris commented Nov 10, 2017

👍

@d-date
Copy link
Contributor Author

d-date commented Nov 13, 2017

@wing328 How about this PR?

@wing328
Copy link
Contributor

wing328 commented Nov 13, 2017

@d-date let me review shortly and get back to you.

@ehyche
Copy link
Contributor

ehyche commented Nov 13, 2017

I'll review tomorrow morning.

@wing328
Copy link
Contributor

wing328 commented Nov 13, 2017

@d-date not related to this PR but the Petstore integration for Swift 4 client is already failing in the latest master. I wonder if you can take a look when you've time: #6944

(swift3 petstore integration tests passed without issue)

@d-date
Copy link
Contributor Author

d-date commented Nov 13, 2017

@wing328 I found that errors related to Codable, which is introduced on Swift4. The swift 4 pet store has little order sample models than generated ones. I’ll try to work on another issue, and fix on another PR, after merged this PR.

@wing328
Copy link
Contributor

wing328 commented Nov 13, 2017

@d-date FYI. We've just merged #6672. Please pull the latest before trying to fix the Swift4 Petstore integration issue.

@d-date
Copy link
Contributor Author

d-date commented Nov 15, 2017

@wing328 finish fixing #6944

@d-date
Copy link
Contributor Author

d-date commented Nov 18, 2017

@wing328 cc @jgavris @ehyche @Edubits @jaz-ah
How about this PR?

@wing328
Copy link
Contributor

wing328 commented Nov 18, 2017

@d-date when I ran ./run_xcodebuild.sh in samples/client/petstore/swift4/default/SwaggerClientTests, I got the following errors:

tended-module-overlay.yaml -Xcc -working-directory/tmp/swagger-codegen/samples/client/petstore/swift4/default/SwaggerClientTests/Pods
<unknown>:0: error: no such file or directory: '/tmp/swagger-codegen/samples/client/petstore/swift4/default/PetstoreClient/Classes/Swaggers/APIs/Fake_classname_tags123API.swift'
<unknown>:0: error: no such file or directory: '/tmp/swagger-codegen/samples/client/petstore/swift4/default/PetstoreClient/Classes/Swaggers/APIs/FakeclassnametagsAPI.swift'
Command /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swiftc failed with exit code 1

** TEST BUILD FAILED **

(this error is different than the one I found in master)
Did you get similar errors when running the script locally?

(I don't think it's caused by this PR but would be nice if you can look into it)

@d-date
Copy link
Contributor Author

d-date commented Nov 18, 2017

@wing328 I found the reason why tests are failed. We cannot use object type in spec as Any in Swift4. The Codable is a kind of Protocol so that we can adopt for struct (f.e. String, Int) types, but Any is primitive, not struct.

As I read below, object is not written in type. How can I handle object in Swift4?
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#data-types

I have alternative as below, but I don't know how to get whether dataType is object in mastache.

before:

open class func testInlineAdditionalPropertiesWithRequestBuilder(param: Any) -> RequestBuilder<Void> {
        let path = "/fake/inline-additionalProperties"
        let URLString = PetstoreClientAPI.basePath + path
        let parameters = JSONEncodingHelper.encodingParameters(forEncodableObject: param)
//Compile Error: cannot convert type
...
    }

After:

open class func testInlineAdditionalPropertiesWithRequestBuilder<T: Encodable>(param: T) -> RequestBuilder<Void> {
        let path = "/fake/inline-additionalProperties"
        let URLString = PetstoreClientAPI.basePath + path
        let parameters = JSONEncodingHelper.encodingParameters(forEncodableObject: param)
    }

@d-date
Copy link
Contributor Author

d-date commented Nov 18, 2017

already issued #6483

@wing328
Copy link
Contributor

wing328 commented Nov 18, 2017

Please get some rest (I believe it's already late in Japan. 3am?) I'll take another look tomorrow

@wing328
Copy link
Contributor

wing328 commented Nov 20, 2017

@jgavris @ehyche @Edubits @jaz-ah would you please take another look at this PR when you've time today?

@wing328 wing328 merged commit a5e4abe into swagger-api:master Nov 21, 2017
@d-date d-date deleted the update-dependencies-swift branch March 12, 2018 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants