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

C#: Constructor is malformed if last property is readOnly: true #2795

Closed
ggood opened this issue May 6, 2016 · 8 comments · Fixed by #2818
Closed

C#: Constructor is malformed if last property is readOnly: true #2795

ggood opened this issue May 6, 2016 · 8 comments · Fixed by #2818

Comments

@ggood
Copy link
Contributor

ggood commented May 6, 2016

Swagger-codegen 2.1.6. With the OpenAPI spec in this Gist:

https://gist.github.com/ggood/099933cdf7aba3cab4867abd500d1b25

the generated c# constructor won't compile, due to a missing argument:

public Foo(string Bar = null, )
        {
            this.Bar = Bar;

        }

This happens when the last property in a definition is readOnly.

@wing328
Copy link
Contributor

wing328 commented May 7, 2016

@ggood I believe I've addressed this in the latest master. I wonder if you can pull the latest master and give it a try.

@jimschubert
Copy link
Contributor

We should probably include a unit test if this is fixed.

I noticed code for required properties last night which I thought might result in this malformed constructor, but I didn't have time to verify my assumption. If this isn't yet fixed, I can grab it early next week.

@wing328
Copy link
Contributor

wing328 commented May 7, 2016

@jimschubert totally agree about adding a test case to cover issues moving forward. I added this before: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml#L807

Let me know that looks good from your perspective (I welcome better and more tests to cover the same issue)

@jimschubert
Copy link
Contributor

@wing328 I think it'd be a great idea to add a sample type to petstore-with-fake-endpoints-models-for-testing.yaml to verify compilation across all languages. I was thinking of a simple unit test case that compiles the yaml definition from this issue and verifies the string output for the class definition to prevent regression. This would be an additional safety against unintentional changes to petstore-with-fake-endpoints-models-for-testing.yaml which would invalidate the test (no longer generating code that would cause the compilation issue).

@wing328
Copy link
Contributor

wing328 commented May 9, 2016

@ggood may I know if you've a chance to try the latest master to see if the issue still persists?

@ggood
Copy link
Contributor Author

ggood commented May 9, 2016

I just did a git pull, mvn clean install, and re-ran the generator on my sample. The generated constructor still looks wrong:

public Foo(string Bar = null, )
        {


                        this.Bar = Bar;

        }

Full constructed class here:

https://gist.github.com/ggood/93a39701368e98e5d59faed56f499857

@wing328
Copy link
Contributor

wing328 commented May 10, 2016

@ggood Thanks for checking. I'll take another look today.

wing328 added a commit to wing328/swagger-codegen that referenced this issue May 10, 2016
@wing328
Copy link
Contributor

wing328 commented May 10, 2016

@ggood it's my fault that my major PR to add better enum support for csharp, php, etc removed part of fix by #2670.

I've added back the fix via #2818.

I'll be more careful rebasing PRs moving forward. Sorry for the inconvenience caused by this.

@jimschubert I misread "snake_case" as the last property but it's not. I've added another property "123Number" (readonly) as the last property and could repeat the issue before the fix so we've this issue covered moving forward. Sorry for the oversight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants