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] Enum parameters are now handled #2640

Merged
merged 1 commit into from
May 19, 2016

Conversation

fabdslv
Copy link
Contributor

@fabdslv fabdslv commented Apr 18, 2016

Fix #2531

@yonaskolb
Copy link
Contributor

This won't compile if multiple operations have the same enum. Maybe you could prepend the operationId to the enum type?

@wing328
Copy link
Contributor

wing328 commented Apr 19, 2016

@fabdslv thanks for the PR.

@yonaskolb thanks for reviewing.

I've a PR for C# and PHP (#2508) and should work for multiple operations with the same enum defined.

I'll try to incorporate your change into my PR.

@yonaskolb
Copy link
Contributor

Well it will break in swift if there are multiple operations with the same tag that share any enums with the same name, as they are in the same scope

@yonaskolb
Copy link
Contributor

The rawValue of the enum will also need to be extracted when added to the path parameters or the nillableParameters, otherwise they will use the printed escaped enum name which may be different from the actual value

@fabdslv
Copy link
Contributor Author

fabdslv commented Apr 19, 2016

@yonaskolb Yes, it might be a good solution to add the operational id. I'm not sure I understand your last comment though, what do you mean by extract the rawValue?

@yonaskolb
Copy link
Contributor

well for the path params instead of
"\({{paramName}})"
use
"\({{paramName}}{{#isEnum}}.rawValue{{/isEnum}})"

and for the query params instead of
"{{baseName}}": {{paramName}}
use
"{{baseName}}": {{paramName}}}{{#isEnum}}{{^required}}?{{/required}}.rawValue{{/isEnum}}

I hope that makes sense. That's what I've done in my custom templates

@wing328
Copy link
Contributor

wing328 commented Apr 19, 2016

@yonaskolb when you've time, do you mind submitting your custom template change with a PR so that we can review and compare? Thanks!

@fabdslv
Copy link
Contributor Author

fabdslv commented Apr 26, 2016

I updated the PR to integrate what @yonaskolb was mentioning. Apart from the fact that it fixes the issues, now the Enum name is {{{datatypeWithEnum}}}_{{operationId}} so that it fixes the conflicts we might encounter.

@wing328 It seems that some changes were made in master that affect my commit and I cannot rebase from master. Could you please take a look at this if you have time? Thanks.


enum for parameter {{paramName}}
*/
public enum {{{datatypeWithEnum}}}_{{operationId}}: String { {{#allowableValues}}{{#values}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is solving the possible clash issue. But for people that don't have a clash, this is created a cumbersome API. Maybe we should add some logic Java-side to know if we have a conflict and do that only in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cjolif I agree with your comment.

@fabdslv I've a pending PR (#2508) that should be able to handle the situation.

@fabdslv
Copy link
Contributor Author

fabdslv commented Apr 26, 2016

@wing328 I managed to rebase, it should be okay now.

@wing328
Copy link
Contributor

wing328 commented Apr 29, 2016

@fabdslv thanks! I'll merge this into #2508 so that we only have javascript and typescript remaining to add better enum support. Hopefully that can be done this weekend...

@wing328 wing328 merged commit 2dda6a6 into swagger-api:master May 19, 2016
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.

4 participants