[swift5] Add useSPMFileStructure #9074

aymanbagabas · 2021-03-25T01:31:07Z

Signed-off-by: Ayman Bagabas [email protected]

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
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.
File the PR against the correct branch: master, 5.1.x, 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

modules/openapi-generator/src/main/resources/swift5/Package.swift.mustache

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java

4brunu · 2021-03-31T10:00:27Z

I left a question, but otherwise looks good to me 👍
Could you please create a new sample project to ensure this works and keep working as expected? Or change a current one to use this flag?
Thanks

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java

samples/client/petstore/swift5/urlsessionLibrary/.openapi-generator/FILES

4brunu · 2021-03-31T16:20:28Z

One last thing that I noticed is that the options USE_SPM_FILE_STRUCTURE and SWIFT_PACKAGE_PATH don't apear in the docs.

To make them apear, you make something like

 cliOptions.add(new CliOption(PROJECT_NAME, "Project name in Xcode"));
 cliOptions.add(CliOption.newBoolean(USE_COROUTINES, "Whether to use the Coroutines adapter with the retrofit2 library."));

4brunu · 2021-03-31T20:01:41Z

@aymanbagabas thanks for you patience!
Looks greats, thanks for taking the time to make this great contribution 👍
If CI passes, this is ready to merge for me 🙂

aymanbagabas · 2021-03-31T21:06:49Z

@4brunu do you have any idea why this is failing? I couldn't tell what I'm missing in the last commit.

[ERROR] Tests run: 1437, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 66.084 s <<< FAILURE! - in TestSuite
[ERROR] checkOptionsProcessing(org.openapitools.codegen.swift5.Swift5OptionsTest)  Time elapsed: 0.019 s  <<< FAILURE!
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Boolean
[ERROR] Failures: 
[ERROR]   Swift5OptionsTest>AbstractOptionsTest.checkOptionsProcessing:51 » ClassCast ja...
[ERROR] Tests run: 1437, Failures: 1, Errors: 0, Skipped: 0
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M3:test (default-test) on project openapi-generator: There are test failures.

4brunu · 2021-04-01T08:32:01Z

Looks like you already fix it.
Can you run the following commands to update the sample projects and docs?
That's why circle ci is failing.

./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh

4brunu · 2021-04-01T16:21:15Z

CI is now passing.
@wing328 for me you can merge this PR

wing328 · 2021-04-03T03:55:44Z

docs/generators/swift5.md

 |swiftUseApiNamespace|Flag to make all the API classes inner-class of {{projectName}}API| |null|
+|useSPMFileStructure|Use SPM file structure and set the source path to Sources/{{projectName}} (default: false).| |null|


@aymanbagabas thanks for taking my feedback.

Shouldn't just swiftPackagePath be enough? In other words, we want to avoid duplicated options do very similar things.

4brunu · 2021-04-05T09:46:20Z

I would like to give you some context regarding this PR.
The current file structure is Classes/OpenAPIs/* because it was like this since the beginning and it’s fine, there is nothing wrong with this.
But then Apple introduced Swift Package Manager(SPM), that is an official package manager for Swift.
With that a standard for the file structure was introduced, Sources/projectName/*.
Since that was introduced, the libraries in the community are being migrated to this new structure.
I think that we should also migrate for this file structure, for good practice reasons.
The only problem is that that change would introduce a breaking change, for people that integrate the generated code manually. For those that use the SPM, Cocoapods or Carthage integration it will work automatically.
I think that a custom path isn’t needed here, because we are implementing a community standard.

My suggestion is that we go with Three of the two options:
1 - Make the breaking change, because this is a community standard, it's very easy to fix, and Swift 5 generator is still in beta.
2 - Make the breaking change, but also introduce a boolean that allows the user go back to the old behaviour.
3 - Don't make the breaking change, by introducing a boolean, and keep the legacy behaviour on by default.

@aymanbagabas @wing328 what do you think of this?

aymanbagabas · 2021-04-05T13:01:37Z

TBH, I'm all for making the breaking change since the generator is still in beta and users don't expect it to be stable. But instead of introducing a boolean to allow the old behavior, users could use swiftPackagePath to set the old behavior back.

wing328 · 2021-04-06T03:31:55Z

Only experimental generators allows breaking changes in the upcoming patch release (5.1.1)

I prefer 3

3 - Don't make the breaking change, by introducing a boolean, and keep the legacy behaviour on by default.

In 5.2.0, we can change the option to use the new behaviour by default.

4brunu · 2021-04-06T08:13:28Z

Looks option 3 also looks good to me.
And what about the option swiftPackagePath should we keep it along side with useSPMFileStructure?
Or should we just keep useSPMFileStructure?
For me I think we can just keep useSPMFileStructure.
@aymanbagabas @wing328 what do you think?

4brunu · 2021-04-06T08:16:13Z

modules/openapi-generator/src/main/resources/swift5/Podspec.mustache

@@ -25,7 +25,7 @@ Pod::Spec.new do |s|
  {{#podDocumentationURL}}
  s.documentation_url = '{{podDocumentationURL}}'
  {{/podDocumentationURL}}
-  s.source_files = '{{projectName}}/Classes/**/*.swift'


@aymanbagabas another place that I remembered that I think we need to update the path is in the XcodeGen.mustache file, the sources property

wing328 · 2021-04-06T16:35:00Z

what do you think?

Prefer one option instead of two for simplicity.

aymanbagabas · 2021-04-06T20:45:45Z

I think this discussion should've taken place earlier. Anyway, since both options have been implemented already and work as expected, I don't see the point of dropping one of them.

As @wing328 mentioned, we can change this behavior and make the breaking changes in the 5.2.0 release. Drop useSPMFileStructure, and set the default source path to Sources/{{projectname}} and use swiftPackagePath to have the old behavior back.

4brunu · 2021-04-07T11:30:53Z

Looks good to me.
@aymanbagabas could you please fix the merge conflicts and the XcodeGen.mustache sources property?

Signed-off-by: Ayman Bagabas <[email protected]>

Prioritize swiftPackagePath over useSPMFileStructure

modules/openapi-generator/src/main/resources/swift5/XcodeGen.mustache

Add useSPMFileStructure to URLSession library

4brunu · 2021-04-08T08:45:55Z

@aymanbagabas thanks for the changes.
Looks good to me.
@wing328 for me this PR is ready to merge 👍

wing328 · 2021-04-21T16:45:03Z

Sorry for the delay. PR merged.

wing328 reviewed Mar 26, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/swift5/Package.swift.mustache Outdated Show resolved Hide resolved

aymanbagabas force-pushed the swift-spmfilestructure branch from 2240406 to 9fdb29b Compare March 30, 2021 18:44

aymanbagabas mentioned this pull request Mar 30, 2021

[swift] Add Swift Vapor client library #8515

Closed

5 tasks

aymanbagabas requested a review from wing328 March 30, 2021 19:39

4brunu reviewed Mar 31, 2021

View reviewed changes

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java Outdated Show resolved Hide resolved

aymanbagabas force-pushed the swift-spmfilestructure branch from 9fdb29b to 2ce916a Compare March 31, 2021 13:47

4brunu reviewed Mar 31, 2021

View reviewed changes

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java Outdated Show resolved Hide resolved

samples/client/petstore/swift5/urlsessionLibrary/.openapi-generator/FILES Show resolved Hide resolved

aymanbagabas force-pushed the swift-spmfilestructure branch 3 times, most recently from 59028cd to 1bf74f5 Compare March 31, 2021 16:00

aymanbagabas force-pushed the swift-spmfilestructure branch 2 times, most recently from 10d6ef5 to 9d9f4b3 Compare March 31, 2021 19:53

aymanbagabas force-pushed the swift-spmfilestructure branch 2 times, most recently from 6272194 to c62c776 Compare March 31, 2021 20:59

aymanbagabas force-pushed the swift-spmfilestructure branch 3 times, most recently from 3601657 to 40077fa Compare April 1, 2021 03:20

aymanbagabas force-pushed the swift-spmfilestructure branch from 40077fa to 21d0705 Compare April 1, 2021 14:38

wing328 reviewed Apr 3, 2021

View reviewed changes

4brunu reviewed Apr 6, 2021

View reviewed changes

aymanbagabas added 4 commits April 7, 2021 09:58

[swift5] Add useSPMFileStructure

1973d5e

Signed-off-by: Ayman Bagabas <[email protected]>

[swift5] Add swiftPackagePath

d60acd7

Prioritize swiftPackagePath over useSPMFileStructure

[swift5] Add cli options and update docs

7b83f94

[swift5] Fix tests

d55d227

aymanbagabas force-pushed the swift-spmfilestructure branch from 21d0705 to 85b6512 Compare April 7, 2021 14:09

4brunu reviewed Apr 7, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/swift5/XcodeGen.mustache Outdated Show resolved Hide resolved

aymanbagabas added 2 commits April 7, 2021 10:46

[swift5] Update XcodeGen source path

cfd2413

[swift5] Update samples and docs

2a86757

Add useSPMFileStructure to URLSession library

aymanbagabas force-pushed the swift-spmfilestructure branch from 85b6512 to 2a86757 Compare April 7, 2021 14:47

wing328 added Client: Swift Enhancement: Feature labels Apr 21, 2021

wing328 added this to the 5.1.1 milestone Apr 21, 2021

wing328 merged commit 3894aa4 into OpenAPITools:master Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[swift5] Add useSPMFileStructure #9074

[swift5] Add useSPMFileStructure #9074

aymanbagabas commented Mar 25, 2021 •

edited

Loading

4brunu commented Mar 31, 2021 •

edited

Loading

4brunu commented Mar 31, 2021

4brunu commented Mar 31, 2021

aymanbagabas commented Mar 31, 2021

4brunu commented Apr 1, 2021

4brunu commented Apr 1, 2021

wing328 Apr 3, 2021 •

edited

Loading

4brunu commented Apr 5, 2021

aymanbagabas commented Apr 5, 2021

wing328 commented Apr 6, 2021

4brunu commented Apr 6, 2021

4brunu Apr 6, 2021

wing328 commented Apr 6, 2021

aymanbagabas commented Apr 6, 2021 •

edited

Loading

4brunu commented Apr 7, 2021

4brunu commented Apr 8, 2021

wing328 commented Apr 21, 2021

		\|swiftUseApiNamespace\|Flag to make all the API classes inner-class of {{projectName}}API\| \|null\|
		\|useSPMFileStructure\|Use SPM file structure and set the source path to Sources/{{projectName}} (default: false).\| \|null\|

[swift5] Add useSPMFileStructure #9074

[swift5] Add useSPMFileStructure #9074

Conversation

aymanbagabas commented Mar 25, 2021 • edited Loading

PR checklist

4brunu commented Mar 31, 2021 • edited Loading

4brunu commented Mar 31, 2021

4brunu commented Mar 31, 2021

aymanbagabas commented Mar 31, 2021

4brunu commented Apr 1, 2021

4brunu commented Apr 1, 2021

wing328 Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

4brunu commented Apr 5, 2021

aymanbagabas commented Apr 5, 2021

wing328 commented Apr 6, 2021

4brunu commented Apr 6, 2021

4brunu Apr 6, 2021

Choose a reason for hiding this comment

wing328 commented Apr 6, 2021

aymanbagabas commented Apr 6, 2021 • edited Loading

4brunu commented Apr 7, 2021

4brunu commented Apr 8, 2021

wing328 commented Apr 21, 2021

aymanbagabas commented Mar 25, 2021 •

edited

Loading

4brunu commented Mar 31, 2021 •

edited

Loading

wing328 Apr 3, 2021 •

edited

Loading

aymanbagabas commented Apr 6, 2021 •

edited

Loading