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

[Issue 9386] Add Swift 5 code generator and templates #9731

Merged
merged 24 commits into from
Oct 26, 2019

Conversation

plam4u
Copy link
Contributor

@plam4u plam4u commented Sep 25, 2019

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 language.

Description of the PR

This PR copies the swift4 code generator and templates to swift5. A new language option for the command-line is registered - swift5. All the compile issues are fixed and the tests are executed to make sure it works. The updates apply to the default implementation only. I haven't touched the RxSwift or ObjC implementations.

Changes done

  • clone all the swift 4 code generators, templates and tests to swift 5
  • fix any compilation errors (update Alamofire dependency and migrate Swift 4.2 to Swift 5.0)

@plam4u plam4u changed the title Issue 9386 [Issue 9386] Add Swift5 code generator Sep 25, 2019
@plam4u
Copy link
Contributor Author

plam4u commented Sep 25, 2019

@HugoMario who is the right person to check this PR?

@plam4u plam4u changed the title [Issue 9386] Add Swift5 code generator [Issue 9386] Add Swift 5 Sep 25, 2019
@plam4u plam4u changed the title [Issue 9386] Add Swift 5 [Issue 9386] Add Swift 5 code generator and templates Sep 25, 2019
This was referenced Sep 25, 2019
@tovkal
Copy link
Contributor

tovkal commented Sep 25, 2019

Great work! I have been waiting for Swift 5 support for so long at #9386. Hopefully this can be merged soon

@HugoMario HugoMario added this to the v2.4.9 milestone Sep 25, 2019
@HugoMario
Copy link
Contributor

thanks a lot, please let me know once you want your changes be merged

@HugoMario
Copy link
Contributor

@plam4u , let me find out who in team can help us with that.

@HugoMario
Copy link
Contributor

hey @4brunu, would you help us reviewing this PR?

s.source_files = '{{projectName}}/Classes/**/*.swift'{{#usePromiseKit}}
s.dependency 'PromiseKit/CorePromise', '~> 4.4.0'{{/usePromiseKit}}{{#useRxSwift}}
s.dependency 'RxSwift', '~> 4.0'{{/useRxSwift}}
s.dependency 'Alamofire', '~> 4.5.0'
Copy link

@4brunu 4brunu Oct 3, 2019

Choose a reason for hiding this comment

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

@plam4u the version of Alomofire included in podspec doesn't compile with Xcode 11 and Swift 5.1.

I needed to upgrade from s.dependency 'Alamofire', '~> 4.5.0' to s.dependency 'Alamofire', '~> 4.9.0'.

Can you also update the podspec please?

Copy link

Choose a reason for hiding this comment

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

@plam4u the version of Alomofire included in podspec doesn't compile with Xcode 11 and Swift 5.1.

I needed to upgrade from 4.5.0 to 4.9.0.

Can you update the podspec please?

}
{{/hasVars}}
{{#additionalPropertiesType}}
public var additionalProperties: [String:{{{additionalPropertiesType}}}] = [:]
Copy link

Choose a reason for hiding this comment

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

This doesn't work with the additionalProperties of the type 'Any' because it outputs two errors.

Protocol type 'Any' cannot conform to 'Encodable' because only concrete types can conform to protocols
Protocol type 'Any' cannot conform to 'Decodable' because only concrete types can conform to protocols

Here is an example of the generated code import Foundation

public struct ProblemDetails: Codable {

public var type: String?
public var title: String?
public var status: Int?
public var detail: String?
public var instance: String?

public init(type: String?, title: String?, status: Int?, detail: String?, instance: String?) {
    self.type = type
    self.title = title
    self.status = status
    self.detail = detail
    self.instance = instance
}
public var additionalProperties: [String:Any] = [:]

public subscript(key: String) -> Any? {
    get {
        if let value = additionalProperties[key] {
            return value
        }
        return nil
    }

    set {
        additionalProperties[key] = newValue
    }
}

// Encodable protocol methods

public func encode(to encoder: Encoder) throws {

    var container = encoder.container(keyedBy: String.self)

    try container.encodeIfPresent(type, forKey: "type")
    try container.encodeIfPresent(title, forKey: "title")
    try container.encodeIfPresent(status, forKey: "status")
    try container.encodeIfPresent(detail, forKey: "detail")
    try container.encodeIfPresent(instance, forKey: "instance")
    try container.encodeMap(additionalProperties) // build error here. `Protocol type 'Any' cannot conform to 'Encodable' because only concrete types can conform to protocols `
}

// Decodable protocol methods

public init(from decoder: Decoder) throws {
    let container = try decoder.container(keyedBy: String.self)

    type = try container.decodeIfPresent(String.self, forKey: "type")
    title = try container.decodeIfPresent(String.self, forKey: "title")
    status = try container.decodeIfPresent(Int.self, forKey: "status")
    detail = try container.decodeIfPresent(String.self, forKey: "detail")
    instance = try container.decodeIfPresent(String.self, forKey: "instance")
    var nonAdditionalPropertyKeys = Set<String>()
    nonAdditionalPropertyKeys.insert("type")
    nonAdditionalPropertyKeys.insert("title")
    nonAdditionalPropertyKeys.insert("status")
    nonAdditionalPropertyKeys.insert("detail")
    nonAdditionalPropertyKeys.insert("instance")
    additionalProperties = try container.decodeMap(Any.self, excludedKeys: nonAdditionalPropertyKeys) // build error here. `Protocol type 'Any' cannot conform to 'Decodable' because only concrete types can conform to protocols`
}

}

One possible solution is using AnyCodable instead of Any

@4brunu
Copy link

4brunu commented Oct 3, 2019

Those two are the issues that I found (I already send them to @plam4u in #9386)

@andreashaese
Copy link

As I wrote elsewhere, could we use Result as the return/callback type instead of (T?, Error?) tuples? I think that would make it even better.

s.source_files = '{{projectName}}/Classes/**/*.swift'{{#usePromiseKit}}
s.dependency 'PromiseKit/CorePromise', '~> 4.4.0'{{/usePromiseKit}}{{#useRxSwift}}
s.dependency 'RxSwift', '~> 4.0'{{/useRxSwift}}
s.dependency 'Alamofire', '~> 4.5.0'
Copy link

Choose a reason for hiding this comment

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

@plam4u the version of Alomofire included in podspec doesn't compile with Xcode 11 and Swift 5.1.

I needed to upgrade from 4.5.0 to 4.9.0.

Can you update the podspec please?

@@ -0,0 +1,3 @@
github "Alamofire/Alamofire" ~> 4.5.0{{#usePromiseKit}}
Copy link

Choose a reason for hiding this comment

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

@plam4u the version of Alomofire included in podspec doesn't compile with Xcode 11 and Swift 5.1.

I needed to upgrade from 4.5.0 to 4.9.0.

Can you also update the Cartfile please?

@4brunu
Copy link

4brunu commented Oct 24, 2019

The command ./bin/swift5-all.sh it's ending with an error:

...
#### Swift4Test Swift API client (default) ####
OpenJDK 64-Bit Server VM warning: ignoring option MaxPermSize=256M; support was removed in 8.0
[main] INFO io.swagger.parser.Swagger20Parser - reading from modules/swagger-codegen/src/test/resources/2_0/swift5Test.json
[main] INFO io.swagger.parser.Swagger20Parser - reading from modules/swagger-codegen/src/test/resources/2_0/swift5Test.json
[main] ERROR io.swagger.parser.SwaggerCompatConverter - failed to read resource listing
java.lang.RuntimeException: Could not find modules/swagger-codegen/src/test/resources/2_0/swift5Test.json on the classpath
	at io.swagger.parser.util.ClasspathHelper.loadFileFromClasspath(ClasspathHelper.java:31)
	at io.swagger.parser.SwaggerCompatConverter.readResourceListing(SwaggerCompatConverter.java:207)
	at io.swagger.parser.SwaggerCompatConverter.read(SwaggerCompatConverter.java:123)
	at io.swagger.parser.SwaggerParser.read(SwaggerParser.java:83)
	at io.swagger.codegen.config.CodegenConfigurator.toClientOptInput(CodegenConfigurator.java:431)
	at io.swagger.codegen.cmd.Generate.run(Generate.java:283)
	at io.swagger.codegen.SwaggerCodegen.main(SwaggerCodegen.java:35)
[main] WARN io.swagger.codegen.ignore.CodegenIgnoreProcessor - Output directory does not exist, or is inaccessible. No file (.swagger-codegen-ignore) will be evaluated.
Exception in thread "main" java.lang.RuntimeException: missing swagger input or config!
	at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:755)
	at io.swagger.codegen.cmd.Generate.run(Generate.java:285)
	at io.swagger.codegen.SwaggerCodegen.main(SwaggerCodegen.java:35)

@HugoMario
Copy link
Contributor

@4brunu, this last error it's something that can be fixed on codegen side. i can help with that

@HugoMario
Copy link
Contributor

what do you guys think about merging this PR now and address new issue in a different tickets? so others developer can propose PR too?

@4brunu
Copy link

4brunu commented Oct 26, 2019

@HugoMario I think the last error it's now fixed with the last commits and my concernes about the Alamofire version are also fixed.
For me you can now merge this PR.

@HugoMario
Copy link
Contributor

oh great, thanks for letting me know @4brunu

@HugoMario HugoMario merged commit fc72545 into swagger-api:master Oct 26, 2019
@HugoMario
Copy link
Contributor

thanks a lot @plam4u !!!!!!!!!!!! for this PR

@plam4u
Copy link
Contributor Author

plam4u commented Oct 27, 2019

Thank you for looking into it, guys!
Thanks to @tovkal for implementing all the last changes and fixes.

haikusw added a commit to haikusw/swagger-codegen that referenced this pull request Nov 3, 2019
It appears that swagger-api#9731 is merged and adds Swift 5 (5.x?) support which it would be nice to call out in the Readme, if accurate.

May want to add @plam4u (https://github.com/plam4u) to the Template Creator list, but I didn't want to do that without checking with them.
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.

None yet

5 participants