Skip to content

[Swift] Some fixes for SwiftGenerator for develop 2.0#901

Merged
wing328 merged 18 commits into
swagger-api:develop_2.0from
tkqubo:feature/swift_develop_2.0
Jul 13, 2015
Merged

[Swift] Some fixes for SwiftGenerator for develop 2.0#901
wing328 merged 18 commits into
swagger-api:develop_2.0from
tkqubo:feature/swift_develop_2.0

Conversation

@tkqubo

@tkqubo tkqubo commented Jun 23, 2015

Copy link
Copy Markdown

No description provided.

@wing328

wing328 commented Jun 23, 2015

Copy link
Copy Markdown
Contributor

@tkqubo thanks for the PR.

I noticed that you're using "/" as path separator in the generator (java). Please kindly replace "/" with File.separator instead otherwise swagger-codegen may likely fail to work in Windows environment.

@tkqubo

tkqubo commented Jun 23, 2015

Copy link
Copy Markdown
Author

@wing328 I see, sorry I completely forgot about Windows environment. I fixed that and pushed again.

@wing328

wing328 commented Jun 23, 2015

Copy link
Copy Markdown
Contributor

@tkqubo no problem at all. Thanks for the quick fix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For suppressRequired, would it be better to obtain it from CLI option? Other generators have used the CLI option approach to obtain additional parameter. To pass additional information to the generator, a config.json (or other json config file) needs to be provided.

Let know if you need more information about the CLI option.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh, as you kindly suggested, I found -c option has been added recently.
Thank you I will switch to Config to store some configuration.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@wing328 Hi, I've updated my pr to fix the issue above

By the way, is it bette to put new commits into a single commit? If so, I will rebase them and push again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tkqubo thanks for the quick turnaround. I'll take another look tomorrow and share with you any feedback I've.

Personally I prefer smaller commits for easier review :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see. I'll squash the commits below 😃

@tkqubo
tkqubo force-pushed the feature/swift_develop_2.0 branch from 8306a20 to 91af76c Compare June 27, 2015 14:24
@wing328

wing328 commented Jul 13, 2015

Copy link
Copy Markdown
Contributor

Did a test and code was generated successfully. The CLI option also looks good.

wing328 added a commit that referenced this pull request Jul 13, 2015
[Swift] Some fixes for SwiftGenerator for develop 2.0
@wing328
wing328 merged commit 045a3b4 into swagger-api:develop_2.0 Jul 13, 2015
@tkqubo

tkqubo commented Jul 13, 2015

Copy link
Copy Markdown
Author

@wing328 Thank you for merging my PR! I will continue to improve swift codegen, though little by little.

@wing328

wing328 commented Jul 13, 2015

Copy link
Copy Markdown
Contributor

@tkqubo thanks again for contribution. Moving forward, we'll try to merge the PR within a week.

@Edubits

Edubits commented Jul 13, 2015

Copy link
Copy Markdown
Contributor

Thanks @tkqubo and @wing328! I was just looking into what's changing in the generator and noticed that in APIs.mustache the class name and basePath are specific to someone's project instead of the placeholders.

Two other things I was wondering about:

  • I want to include the generated code into a Framework (through CocoaPods), some changes has to be made for this. Most importantly all methods and classes which should be accessible need to be annotated with public. As far as I know this won't have an effect on other uses, shall I create a PR for this?
  • In api.mustache the API classes are inside an extension of {{projectName}}API, how does this to work? I had to remove this to get it running.

@wing328

wing328 commented Jul 13, 2015

Copy link
Copy Markdown
Contributor

For 1, there's a PR (#918) to add packaging for ObjC SDKs. I wonder if you can do a test and confirm whether you're looking for something similar in the Swift API client.

For 2, you can pass additional option via a JSON file, e.g. -c /var/tmp/config.json. Here are the complete options available to the Swift client:

$ java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar config-help -l swift

CONFIG OPTIONS
    modelPackage
        package for generated models

    apiPackage
        package for generated api classes

    projectName
        Project name in Xcode

    responseAs
        Optionally use libraries to manage response.  Currently PromiseKit are available.

    unwrapRequired
        Treat 'required' properties in response as non-optional (which would crash the app if api returns null as opposed to required option specified in json schema

Would this help ?

@Edubits

Edubits commented Jul 13, 2015

Copy link
Copy Markdown
Contributor

Thanks for the quick response.

For 1, there's a PR (#918) to add packaging for ObjC SDKs. I wonder if you can do a test and confirm whether you're looking for something similar in the Swift API client.

This is indeed related, however because of the differences between Swift and Objc and how CocoaPods has to handle them the solution will be different. (CocoaPods with Swift uses frameworks instead of static libraries. And they need explicit access annotations, where in Objc the header files are used)

For 2, you can pass additional option via a JSON file, e.g. -c /var/tmp/config.json. Here are the complete options available to the Swift client:

This is not related, maybe I wasn't clear. What I'm referring to is the code on api.mustache line 11:
extension {{projectName}}API {
In my understanding this line (and of course the matching closing bracket) shouldn't be there.

@wing328

wing328 commented Jul 13, 2015

Copy link
Copy Markdown
Contributor

@Edubits thanks for the clarification. I'll defer to @tkqubo to comment further.

@tkqubo

tkqubo commented Jul 14, 2015

Copy link
Copy Markdown
Author

@Edubits Thanks for your questions and proposals!

the class name and basePath are specific to someone's project instead of the placeholders.

Oh that was a huge mistake! Sorry I have to fix it immediately 😰

And for 1, sorry could you create pull request for this?

For 2, I intended to use the class {{projectName}}API as a namespace, which does not work because of the bug you pointed above. After I revert the lines below, the code should compile. (Although there is a debate whether this namespace packaging is needed or not)

91af76c#diff-461948efc36982a4977ee49841d661d5L10

@tkqubo

tkqubo commented Jul 14, 2015

Copy link
Copy Markdown
Author

And @wing328 Thank you so much for your kind explanation!

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.

3 participants