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

[Swift] Fix unwrapRequired behavior so it actually works #1527

Closed
wants to merge 6 commits into from

Conversation

joeboyscout04
Copy link
Contributor

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.
    @clasnake @jimschubert @shijinkui @ramzimaalej

Description of the PR

Previously the unwrapRequired config option didn't exist for swift4 generator (see #228), but the previous PR (#229) didn't fully fix the issue due to some faulty logic and a missing default value. That has now been corrected so this option should work again.

@@ -59,7 +59,7 @@
protected static final String LIBRARY_RX_SWIFT = "RxSwift";
protected static final String[] RESPONSE_LIBRARIES = {LIBRARY_PROMISE_KIT, LIBRARY_RX_SWIFT};
protected String projectName = "OpenAPIClient";
protected boolean unwrapRequired;
protected boolean unwrapRequired = true;
Copy link
Member

Choose a reason for hiding this comment

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

@joeboyscout04 thanks for the fix. Since the default was "false", we'll need to target this in 4.0.x (upcoming major release)

If you change it back to "false" as the default, we can merge it into master (which only allows backward-compatible changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm misunderstanding what this is supposed to do, it is already unwrapping the required parameters by default. I think this option never actually did anything.

Copy link
Member

Choose a reason for hiding this comment

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

I mean unwrapRequired defaults to false at the moment. Changing the default setting of unwrapRequired to "true" will impact users who do not need this feature.

(I didn't say your PR does not fix the issue and I agree with you the option may have been broken as it's not switched on by default)

@wing328
Copy link
Member

wing328 commented Nov 23, 2018

@joeboyscout04 thanks for the PR. If I understand correctly, is the unwrapRequired option same as or very simliar to the nullable feature in OAS3?

We've start adding nulalble support to the generators:

so maybe we can use nullable attribute in the spec to replace the option "unwrapRequired"

In OAS2, one can use x-nullable vendor extension instead.

@wing328
Copy link
Member

wing328 commented Dec 28, 2018

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

@fl034
Copy link
Contributor

fl034 commented Jan 31, 2019

@wing328 I really hope that this gets into 4.0 or a 3.4 bugfix release. Having models with all properties as optional (nullable) is a big misuse of the programming language feature.

I currently have multiple projects running on API client code generated by generator of this pull request...

@wing328
Copy link
Member

wing328 commented Jan 31, 2019

@joeboyscout04 we definitely want to include your fix.

I'll try to update your PR to resolve the merge conflicts later today or tomorrow. Thanks again for the contribution.

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

@wing328
Copy link
Member

wing328 commented Aug 30, 2019

I've filed #3783 to use OAS v3 nullable (or x-nullable in OAS v2) instead.

@wing328
Copy link
Member

wing328 commented Jan 2, 2020

The fix is available in the newly-added swift5 generator so closing this one instead.

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