[Python] fix numeric enum in python flask, aiohttp #5019

wing328 · 2020-01-17T07:01:28Z

Plotted the fix from Python client generator
Tested locally with numeric enum

cc @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @slash-arun (2019/11) @spacether (2019/11)

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

spacether · 2020-01-17T17:10:16Z

modules/openapi-generator/src/main/resources/python-flask/model.mustache

@@ -116,6 +118,7 @@ class {{classname}}(Model):
 {{/isMapContainer}}
 {{/isContainer}}
 {{^isContainer}}
+        allowed_values = [{{#isNullable}}None,{{/isNullable}}{{#allowableValues}}{{#values}}{{#isString}}"{{/isString}}{{{this}}}{{#isString}}"{{/isString}}{{^-last}}, {{/-last}}{{/values}}{{/allowableValues}}]  # noqa: E501


We are tying to minimize the usage of flake8 E501 and other flake8 opt out lines.
Can you spread this to a multiline statement so we won't need # noqa: E501?

spacether · 2020-01-17T17:11:58Z

modules/openapi-generator/src/main/resources/python-aiohttp/model.mustache

 {{#isContainer}}
+        allowed_values = [{{#isNullable}}None,{{/isNullable}}{{#allowableValues}}{{#values}}{{#items.isString}}"{{/items.isString}}{{{this}}}{{#items.isString}}"{{/items.isString}}{{^-last}}, {{/-last}}{{/values}}{{/allowableValues}}]  # noqa: E501


We are tying to minimize the usage of flake8 E501 and other flake8 opt out lines.
Can you spread this to a multiline statement so we won't need # noqa: E501?

spacether · 2020-01-17T17:12:06Z

modules/openapi-generator/src/main/resources/python-aiohttp/model.mustache

@@ -109,6 +109,7 @@ class {{classname}}(Model):
 {{/isMapContainer}}
 {{/isContainer}}
 {{^isContainer}}
+        allowed_values = [{{#isNullable}}None,{{/isNullable}}{{#allowableValues}}{{#values}}{{#isString}}"{{/isString}}{{{this}}}{{#isString}}"{{/isString}}{{^-last}}, {{/-last}}{{/values}}{{/allowableValues}}]  # noqa: E501


We are tying to minimize the usage of flake8 E501 and other flake8 opt out lines.
Can you spread this to a multiline statement so we won't need # noqa: E501?

spacether · 2020-01-17T17:12:20Z

modules/openapi-generator/src/main/resources/python-flask/model.mustache

 {{#isContainer}}
+        allowed_values = [{{#isNullable}}None,{{/isNullable}}{{#allowableValues}}{{#values}}{{#items.isString}}"{{/items.isString}}{{{this}}}{{#items.isString}}"{{/items.isString}}{{^-last}}, {{/-last}}{{/values}}{{/allowableValues}}]  # noqa: E501


We are tying to minimize the usage of flake8 E501 and other flake8 opt out lines.
Can you spread this to a multiline statement so we won't need # noqa: E501?

Let's do this in a separate PR instead. I'll open an issue to track it to see if anyone has the time to improve it.

(this PR simply plots the fix from the python client template)

Are you suggesting that we remove the E501s in a separate PR? That sounds good.

Created #5030 for tracking.

spacether · 2020-01-17T17:17:18Z

samples/server/petstore/python-aiohttp/openapi_server/openapi/openapi.yaml

@@ -32,7 +32,7 @@ paths:
        required: true
        x-body-name: body
      responses:
-        405:
+        "405":


These spec updates are unexpected in this pull review because we did not edit the spec file.
Can you please add the below sample generation files to https://github.com/OpenAPITools/openapi-generator/blob/master/bin/utils/ensure-up-to-date?

python-aiohttp

python-flask-python2

python-flask

Added to ensure-up-to-date.

These scripts are using petstore.yaml (original) but not the fake petstore spec (which has numeric enums)

Can we add a model with numeric enums in petstore.yaml?
That lets us verify this fix in the repo and this PR. Or a java test like I mention below also verifies the fix.

We can update the script to use the fake petstore spec instead (petstore.yaml shouldn't be updated)

When a generator gets mature, we will use fake petstore spec instead of petstore.yaml/json as the generator should be able to handle those edge cases in fake petstore spec.

I'm not sure if these generators can handle the edge cases in fake petstore spec.

spacether · 2020-01-17T17:29:20Z

Tested locally with numeric enum

I am seeing that our sample spec has numeric enums, including numeric enums in models.
Why are we not seeing a model update including this case?

If we don't have a spec of this use case, could you add a minimal spec with it?

Adding this spec will also allow other generators to test for this condition.

If we do have a spec with this use case can you use it?
Can you add JAVA test here with that spec using one of fromModel/fromOperation/fromProperty etc verifying that the generated enum is correct?

wing328 · 2020-01-19T05:46:04Z

Can you add JAVA test here with that spec using one of fromModel/fromOperation/fromProperty etc verifying that the generated enum is correct?

If you look at the fix, it cannot be tested in the Java class because it uses isString in the mustache template to determine whether a double quote is required.

Later after switching these scripts from petstore.yaml to fake petstore petstore, we can add test(s) in the samples to ensure the output is correct.

spacether

Good point about the testing. This looks good as is.

* master: Use HTTPS Maven URL in Kotlin meta generator (#5043) [Python] fix numeric enum in python flask, aiohttp (#5019) add multipleOf to various codegen classes (#5021) Add a link to a youtube video on Pulp and OpenAPI Generator (#5038)

* 5.0.x: (92 commits) [Rust Server] Handle array of objects inside an object correctly (#5044) [Rust Server] Sanitise enum values (#5042) [Rust Server] Add support for primitive arrays (#5041) [elm] Fix duplicate coder names (#5100) [elm] enum items and parameters (#5051) [elm] Fix decoding empty operation responses (#5055) resolve merge conflicts in VERSION files undo restoring ElmClientCodegenTest.java fix merge conflicts [dart-dio] Various fixes (#5027) Add java code comments based on explanation from Jim Schubert (#5048) Execute ./bin/openapi3/go-experimental-petstore.sh script (#5049) [docs] Sorted doc outputs and clean up duplicated CliOptions (#5046) Add docs/installation.md and docsite index to version update script (#5037) [gradle] consistent use of maven url in gradle files (#5045) Use HTTPS Maven URL in Kotlin meta generator (#5043) [Python] fix numeric enum in python flask, aiohttp (#5019) add multipleOf to various codegen classes (#5021) Add a link to a youtube video on Pulp and OpenAPI Generator (#5038) [scripts] Remove misspelled OPENAPI_GENERATOR_DOWLOAD_CACHE_DIR (#5035) ...

wing328 added 2 commits January 17, 2020 14:41

minor code format fix

432d060

fix numeric enum in python flask and aiohttp

d789278

wing328 added this to the 4.2.3 milestone Jan 17, 2020

wing328 added Feature: Enum Issue: Bug Server: Python labels Jan 17, 2020

spacether reviewed Jan 17, 2020

View reviewed changes

add python-server-all.sh to ensure-up-to-date

07e39fb

wing328 mentioned this pull request Jan 18, 2020

[REQ] Put the enum values in multiple lines to avoid using E501 #5030

Closed

spacether approved these changes Jan 19, 2020

View reviewed changes

wing328 merged commit fff759b into master Jan 19, 2020

wing328 deleted the python_flask_fix_enum branch January 19, 2020 07:25

wing328 mentioned this pull request Jan 19, 2020

[Python][Server] add tests for enums (string, numeric) #5039

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] fix numeric enum in python flask, aiohttp #5019

[Python] fix numeric enum in python flask, aiohttp #5019

wing328 commented Jan 17, 2020 •

edited

Loading

spacether Jan 17, 2020

spacether Jan 17, 2020

spacether Jan 17, 2020

spacether Jan 17, 2020

wing328 Jan 18, 2020

spacether Jan 18, 2020 •

edited

Loading

wing328 Jan 18, 2020

spacether Jan 17, 2020

wing328 Jan 18, 2020

spacether Jan 18, 2020 •

edited

Loading

wing328 Jan 18, 2020 •

edited

Loading

spacether commented Jan 17, 2020 •

edited

Loading

wing328 commented Jan 19, 2020

spacether left a comment

		{{#isContainer}}
		allowed_values = [{{#isNullable}}None,{{/isNullable}}{{#allowableValues}}{{#values}}{{#items.isString}}"{{/items.isString}}{{{this}}}{{#items.isString}}"{{/items.isString}}{{^-last}}, {{/-last}}{{/values}}{{/allowableValues}}] # noqa: E501

[Python] fix numeric enum in python flask, aiohttp #5019

[Python] fix numeric enum in python flask, aiohttp #5019

Conversation

wing328 commented Jan 17, 2020 • edited Loading

PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

wing328 Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

spacether commented Jan 17, 2020 • edited Loading

wing328 commented Jan 19, 2020

spacether left a comment

Choose a reason for hiding this comment

wing328 commented Jan 17, 2020 •

edited

Loading

spacether Jan 18, 2020 •

edited

Loading

spacether Jan 18, 2020 •

edited

Loading

wing328 Jan 18, 2020 •

edited

Loading

spacether commented Jan 17, 2020 •

edited

Loading