[Python] improved class reflection in deserialization and better model construction with mandatory attributes

[easz]

PR checklist

• Read the contribution guidelines.
• Ran the shell/batch 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)
• Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

try to improve python client generation.

• model construction with mandatory attributes
• improve class reflection in deserialization
• test (top-level) enum class deserialization

See
#5375
#5421

[wing328]

Thanks for the PR.

cc @frol @scottrw93 @taxpon @baartosz

@easz btw, have you checked out the 2.3.0 branch in which Python API client has a lot of enhancements (with breaking changes)?

[easz]

$ cd swagger-codegen/

$ git checkout remotes/swagger-api/2.3.0

// the last commit c01cb9e of this PR is not part of changes
$ git cherry-pick ca45e6f 5b5c2f6 5bb87a2 89d8bd4 c40b7dc 8b584d2 c64c7ad

// delete 'test' for new code
// the 'test' folder will not be overwritten if already exist. Desired behavior?
$ rm -rf samples/client/petstore/python/test

$ ./bin/python-petstore.sh

$ cd samples/client/petstore/python/
$ ./test_python2_and_3.sh

Ran 100 tests in 11.110s

OK (SKIP=1)
________________________________________________________ summary ________________________________________________________
py27: commands succeeded
py3: commands succeeded
congratulations :)

[wing328]

// the 'test' folder will not be overwritten if already exist. Desired behavior?

Yes, that's the expected behaviour because we do not want to overwrite the test files as the users might have manually added a few test cases in the test files.

[easz]

that means
./bin/python-petstore.sh will never update 'test' folder even 'model_test.mustache' is changed?

actually I did assume 'test' folder is automated generated tests
and 'tests' folder has user defined tests.

[wing328]

./bin/python-petstore.sh will never update 'test' folder even 'model_test.mustache' is changed?

Yes, that's the current behaviour.

actually I did assume 'test' folder is automated generated tests
and 'tests' folder has user defined tests.

That's one way to do it.

We plan to leverage .swagger-codegen-ignore to control which files not be overwritten during code generation.

[wing328]

@easz if I understand correctly, these changes are backward-compatible with the existing auto-genreated Python API using the latest master.

I'll merge this PR into master on coming Wed if no one has any further feedback.

[frol]

I think, this should be added locally on your own machine to the global git ignore file: https://davidwalsh.name/global-gitignore

[easz]

yes. the change (e.g. required attributes need to be defined when calling model constructor) is backward-compatible with the existing auto-genreated Python API.

but some test cases has been changed to compatible with the change.

and any existing client code by any user may break if they did not assign required attributes in the constructor.

[frol]

The change looks great to me.

P.S. I have noticed that there are some places where 2 spaces are used for indentation while 4 spaces indentation is recommended. Is this worth to update in the scope of this PR?

[wing328]

Is this worth to update in the scope of this PR?

I don't think so. I'll create another issue to track the code format of Python API client similar to what we've done for the PHP API client (refactored): #5277

[wing328]

and any existing client code by any user may break if they did not assign required attributes in the constructor.

@easz It's definitely the correct behaviour and I would imagine some applications using the upgraded Python API client will throw exception.

May I know what the exception message looks like? If it's clear, I think it's fine for this enhancement to go into the current master.

[easz]

a ValueError, if a required attribute is not given in a model constructor

Exception is thrown from files generated by model.mustache

  if {{name}} is None:
    raise ValueError("Invalid value for `{{name}}`, must not be `None`")

before the change, the test cases under test/ just construct every possible model with null content

[wing328]

@easz the error message looks good to me. We might revise it later to make it consistent with the error message shown by other API clients.