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

[swift3] empty model with only additional properties #6273

Merged
merged 1 commit into from
Aug 11, 2017

Conversation

wuf810
Copy link
Contributor

@wuf810 wuf810 commented Aug 9, 2017

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 for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

When a class is defined as having no variables but does have additionalProperties, the class would be created incorrectly as a broken typeAlias.

In addition if a class is defined with no variables and no additionalProperties a broken typeAlias was generated. This is now creates an empty jsonencodable class.

This fixes issue #5535

cc @dwoodel

@wuf810
Copy link
Contributor Author

wuf810 commented Aug 9, 2017

Hey is the travis error something we've done or does the test sometimes fail?

@wing328

@wuf810 wuf810 force-pushed the empty-vars-but-props branch from fa43b61 to 70a95e0 Compare August 9, 2017 18:50
@wuf810
Copy link
Contributor Author

wuf810 commented Aug 9, 2017

My bad, my local master was one behind which maybe why the tests failed. It is now fixed.

@wuf810
Copy link
Contributor Author

wuf810 commented Aug 9, 2017

OK same question…any idea why the travis job might have failed?

@wuf810 wuf810 mentioned this pull request Aug 10, 2017
3 tasks
@wing328
Copy link
Contributor

wing328 commented Aug 10, 2017

@wuf810 I'll take a look.

@wing328
Copy link
Contributor

wing328 commented Aug 10, 2017

Tests with updated Petstore samples (swift, swfit3) passed via https://travis-ci.org/swagger-api/swagger-codegen/builds/263164376?utm_source=email&utm_medium=notification

@wing328
Copy link
Contributor

wing328 commented Aug 11, 2017

cc @jaz-ah @jgavris @Edubits

@jgavris
Copy link
Contributor

jgavris commented Aug 11, 2017

This is great, I've actually been running this on a private fork for a while. I wasn't sure the implications of it for others so I didn't PR it.

@jgavris
Copy link
Contributor

jgavris commented Aug 11, 2017

LGTM

@jgavris
Copy link
Contributor

jgavris commented Aug 11, 2017

@wing328 What's the status with the github rate limit on the typescript thing?

@wing328
Copy link
Contributor

wing328 commented Aug 11, 2017

What's the status with the github rate limit on the typescript thing?

@jgavris The latest master should have fixed it.

@jgavris
Copy link
Contributor

jgavris commented Aug 11, 2017

Sorry, which commit though? This PR failed on that.

@wing328
Copy link
Contributor

wing328 commented Aug 11, 2017

Sorry, which commit though? This PR failed on that.

Should be fixed by #6178

Let me merge this one into master to see if I could repeat the Github rate limit error (I've not seen this error myself for a while)

@wing328 wing328 merged commit c2ababb into swagger-api:master Aug 11, 2017
@wing328
Copy link
Contributor

wing328 commented Aug 11, 2017

@wuf810 thanks for the PR, which has been merged into master.

When you've time, I wonder if you can make the same enhancement to Swift4 generator.

cc @ehyche

@wing328
Copy link
Contributor

wing328 commented Aug 12, 2017

UPDATE about the Github rate limit error: it's my oversight and I've commented out the test in Travis for the time being until someone has cycle to replace tsd with typings in the TS Angular client.

@wuf810
Copy link
Contributor Author

wuf810 commented Aug 15, 2017

Of course, I'll take a look at the Swift4 later in the week.

@wuf810
Copy link
Contributor Author

wuf810 commented Sep 12, 2017

@wing328 @ehyche My apologies but I've not had time to apply the same enhancement to the Swift4 generator.

I'm currently recovering from surgery so not sure when I am able to look.

Could one of you do this for me? Or do you want to wait on me?

@ehyche
Copy link
Contributor

ehyche commented Sep 12, 2017

@wuf810 : No worries. I am working on adding this functionality to the swift4 generator. PR should be up shortly.

@ehyche
Copy link
Contributor

ehyche commented Sep 13, 2017

@wuf810 @wing328 : I implemented this functionality for swift4 in this PR I just filed: #6492

@wuf810
Copy link
Contributor Author

wuf810 commented Sep 13, 2017

Brilliant. Thank you.

@wing328
Copy link
Contributor

wing328 commented Sep 13, 2017

@wuf810 can you please help test the PR when you've time?

git checkout -b ehyche-ehyche_swift4_empty_model_only_additional_properties master
git pull https://github.com/ehyche/swagger-codegen.git ehyche_swift4_empty_model_only_additional_properties
mvn clean package
# generate swift client for testing

@wing328
Copy link
Contributor

wing328 commented Sep 18, 2017

@wuf810 the enhancement has been merged into master. Please pull the latest to give it a try.

@wuf810
Copy link
Contributor Author

wuf810 commented Sep 22, 2017

@wing328 Sorry still not fully recovered from surgery. However I have managed to have a quick look at the latest master and generated our client code and so far everything looks good.

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.

5 participants