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 #1571 - Improved objc Compatibility #1599

Closed

Conversation

Sal0g
Copy link

@Sal0g Sal0g commented Dec 3, 2018

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: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)

Description of the PR

PR to improve the Objc compatibility, see Issue #1571

Sal0g added 2 commits December 1, 2018 12:08
Improved Objective C compatibility when using **objcCompatible**
- Use **class** instead of **struct**
- Add **@objc** and **NSObject** for classes in API and model classes
@wing328
Copy link
Member

wing328 commented Dec 3, 2018

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@Sal0g
Copy link
Author

Sal0g commented Dec 3, 2018

That is fine for me.

However if you need me to fix it I will look into it.

- Models: Expose non-optional variables to @objc. Otherwise e.g. Int64 is not visible in objc
- Models: Try avoiding generated code that has boolVariable.map ({ return NSNumber(value: $0) }) as map does not exist for Bool in Swift (see Issue OpenAPITools#1456)
- API: Avoid Void? arguments with @objc, as they are not available in objc
{{^returnType}}
{{^returnType}}{{#objcCompatible}}
completion(error)
{{/objcCompatible}}{{^objcCompatible}}
Copy link
Member

Choose a reason for hiding this comment

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

@Sal0g you may want to use the following instead to avoid trailing/leading spaces or empty lines:

        {{^returnType}}
        {{#objcCompatible}}
        completion(error)
        {{/objcCompatible}}
        {{^objcCompatible}}
        ...
        {{/objcCompatible}}
        {{/returnType}}

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thanks! Considered this in latest commit.

Cosmetic changes to avoid trailing/leading spaces or empty lines.
@Sal0g
Copy link
Author

Sal0g commented Dec 14, 2018

I have updated the branch and added include part fixes to #1456 as they are connected. As far as I understand, I do not need create a new PR?

@wing328 wing328 modified the milestones: 4.0.0, 4.0.1 May 13, 2019
@wing328 wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019
@wing328 wing328 modified the milestones: 4.0.2, 4.0.3 Jun 20, 2019
@wing328 wing328 modified the milestones: 4.0.3, 4.1.0 Jul 9, 2019
@wing328 wing328 modified the milestones: 4.1.0, 4.1.1 Aug 9, 2019
@wing328 wing328 modified the milestones: 4.1.1, 4.1.2 Aug 26, 2019
@wing328 wing328 modified the milestones: 4.1.2, 4.1.3 Sep 11, 2019
@wing328 wing328 modified the milestones: 4.1.3, 4.2.0 Oct 4, 2019
@wing328 wing328 removed this from the 4.2.0 milestone Oct 30, 2019
@4brunu
Copy link
Contributor

4brunu commented Dec 25, 2019

@Sal0g Thanks for your PR! Are you still interested in merging this PR?

The objc interoperability is currently broken, and your PR improves it.

I have tested it and the generated code is better for objc interoperability, but the build on the sample project for objc interoperability fails with the changes.

The two issues I found are related to methods marked with the @objc annotation that have parameters that are not eligible in objc like Int?, and models that contains properties with the same issue described before.

To see the issue can you please merge master into your branch and run the following command on the terminal from the root directory of openapi-generator?

./mvnw package && sh bin/swift4-petstore-objcCompatible.sh && ./mvnw -f samples/client/petstore/swift4/objcCompatible/pom.xml integration-test

This command will build openapi-generator with your changes, generate the objc interoperability project sample and build it.

@Sal0g
Copy link
Author

Sal0g commented Jan 2, 2020

Hi! Yes I definitiv want to merge this PR and I will do this in the following days as I am currently busy.

@4brunu
Copy link
Contributor

4brunu commented Jan 2, 2020

When you have time 🙂

You may want to make this changes in the new Swift5 generator that just got merged. #4086

By the way, I look a into the objc compatibility problem and made some progress, but it's not yet there, so if this can somehow help you to finish this, here is the things I tried. https://github.com/4brunu/openapi-generator/tree/feature/swift5-objc-compatible

@4brunu
Copy link
Contributor

4brunu commented Mar 2, 2020

Since #5410 was merged, I think this PR can now be closed.

#5410 doesn't fix everything related to objc compatibility, but it makes some progress.

The remaining issues are the variables that are:

  • The methods in the API class need to be marked with @objc
  • The parameters passed in to API class methods sometimes are not directly representable in objc like int? that needs to be converted to NSNumber?
  • The model properties are not marked as @objc

@wing328
Copy link
Member

wing328 commented Mar 2, 2020

Agreed with closing this for the time being. If anyone wants to reopen, please reply to let us know.

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.

3 participants