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

Fix alias to map in the model's properties #360

Merged
merged 14 commits into from
Jun 21, 2018
Merged

Fix alias to map in the model's properties #360

merged 14 commits into from
Jun 21, 2018

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Jun 20, 2018

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.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

To fix #255, #191

This is the first fix for "alias". Please expect more PRs to fix various usage of alias (e.g. alias of alias, alias of array, alias handling in response, parameters, etc)

In the upcoming PRs, I'll also add more Java unit tests to cover the issue moving forward.

@@ -64,7 +64,7 @@ def self.openapi_types
:'enum_string_required' => :'String',
:'enum_integer' => :'Integer',
:'enum_number' => :'Float',
:'outer_enum' => :'OuterEnum'
:'outer_enum' => :'String'
Copy link
Member Author

Choose a reason for hiding this comment

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

@ackintosh FYI. Now outer_enum is referencing String correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

The PHP tests failed with the following errors:

PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

.........................................F....................... 65 / 69 ( 94%)
....

Time: 2.12 seconds, Memory: 15.75MB

There was 1 failure:

1) OpenAPI\Client\OuterEnumTest::testSanitizeNestedInvalidValue
Failed asserting that exception of type "InvalidArgumentException" is thrown.

I tried to fix it locally but in vain. I tried supplying invalid values (e.g. "not_upper", 1000, etc to other enum values) but still no exception is thrown.

I've skipped the test via d88c9bf for the time being

Copy link
Member Author

Choose a reason for hiding this comment

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

UPDATE: I pushed another commit to retain the top-level enum classes. Thanks for the test case to cover it.

@wing328
Copy link
Member Author

wing328 commented Jun 20, 2018

cc core team (@jimschubert @jmini @cbornet @ackintosh @JFCote @jaz-ah )

@jmini
Copy link
Member

jmini commented Jun 21, 2018

I did not look at it in details yet, it looks good to me in general.


One question about the targeted branch:

this change has a big impact on the generated code (it is an improvement because it solves something that is not correct now, but still an impact). This might break some existing code relying on the generated code.

I think it is preferable to have this filed for 3.1.x (just change the target branch in the GitHub UI). From a release perspective, it is only one week more to wait (3.1.0 is due at beginning of july).

@wing328
Copy link
Member Author

wing328 commented Jun 21, 2018

I think it is preferable to have this filed for 3.1.x (just change the target branch in the GitHub UI). From a release perspective, it is only one week more to wait (3.1.0 is due at beginning of july).

Definitely a valid point. I don't mind targeting 3.1.x. I'll update it later today if no one has other suggestions/feedbacks on which branch this PR should target.

@wing328 wing328 changed the base branch from master to 3.1.x June 21, 2018 08:15
@wing328 wing328 closed this Jun 21, 2018
@wing328 wing328 reopened this Jun 21, 2018
@etherealjoy
Copy link
Contributor

Issue solved for #255
With the spec mentioned in this issue, the code is generated for go-server and aspnetcore.

@wing328
Copy link
Member Author

wing328 commented Jun 21, 2018

The CI failures are not related to this change

@wing328 wing328 merged commit a897fee into 3.1.x Jun 21, 2018
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.

None yet

3 participants