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

Adding model properties or parameters with "enum" constraint to the codeModel as EnumType with modelAsString: true #8

Merged
merged 1 commit into from
Sep 19, 2017

Conversation

amarzavery
Copy link
Contributor

  • We were previously treating such model properties and parameters as enums with ModelAsString true anyway. This PR adds them to the codeModel thus making the circle complete.
  • Once the PR is merged, codegenerators taking a dependency on the modeler with this this change will see an extra model in their library (For example: A static class with constant properties in case of C#)
  • There should be no breaking changes to the generated client.

…odeModel as EnumType with modelAsString: true
@msftclas
Copy link

@amarzavery,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@@ -341,7 +341,7 @@ public void TestDataTypes()
Assert.Equal("Double double", CreateCSharpDeclarationString(codeModel.Methods[0].Parameters[5]));
Assert.Equal("Decimal decimal", CreateCSharpDeclarationString(codeModel.Methods[0].Parameters[6]));
Assert.Equal("String string", CreateCSharpDeclarationString(codeModel.Methods[0].Parameters[7]));
Assert.Equal("enum color", CreateCSharpDeclarationString(codeModel.Methods[0].Parameters[8]));
Copy link
Contributor

Choose a reason for hiding this comment

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

wow how was enum color ever a valid declaration... I love our tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We simply wanted tests to pass.. ROFL

@olydis
Copy link
Contributor

olydis commented Sep 19, 2017

thanks for the nice commenting :) 👍

@olydis olydis merged commit e83172c into Azure:master Sep 19, 2017
@amarzavery
Copy link
Contributor Author

I found more issues after using the updated modeler locally with the TS generator.
In our test swaggers at lot of places we are using the swagger enums "enum": [..] without the x-ms-enum extension. These enum contain weird values for example:

  • An empty string enum "enum": [""] generates in TS enum Foo { = "" } which is invalid. The member name should be sanitized to EMPTY_STRING.
    Other cases were of strings like /http/put/201; where slashes make the member name invalid.

One case was of enum containing MBCS encoding chinese characters.

We can sanitize them in the modeler or do this in every generator.

@olydis
Copy link
Contributor

olydis commented Sep 19, 2017

oh dear... there actually is a sanitation method somewhere but not sure who calls it and when - I would have expected it to at least handle the slashes. But yeah, empty string and non-ASCII characters should be sanitized too, ideally by extending that function

Copy link
Member

@fearthecowboy fearthecowboy left a comment

Choose a reason for hiding this comment

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

LGTM +1

We should generate a whole bunch of the rest-specs and see if there is any problems.

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.

4 participants