[python-experimental] automatically use values for enums of length1

[spacether]

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
• Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
  @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01)

Description of the PR

This PR updates the function signatures for models and endpoints
FROM
(req_arg1=None, req_arg2_with_default=None, optional_arg1=None):
TO:
(req_arg1, req_arg2_with_default=default_value, optional_arg1=None):
So required arguments without defaults are now positional
Required arguments with defaults are now named arguments which use the default value

• If a variable has an enum of length one, we load that value into the default value

This is helpful because:

• it clarifies from the function signature what is required versus optional
• we are now automatically using enums of length one if the spec writer defines them

Note about invoking functions with positional args:
In python2 and python3 if a function has a signature:

def func(arg1, arg2=None):
  pass
# one can invoke it with
func('arg1_val', arg2='blah')
# OR
func(arg1='arg1_val', arg2='blah')

So if people are invoking these functions with keyword arguments, they will continue working.

Positional vs Keyword Arguments:

If we do not like making these required arguments positional, and we want to keep them as keyword arguments, I can:

• change the default values for them back to None
• if not Nullable, throw an error if None is passed in on that variable

This PR also adds a windows tool at bin/windows/python-experimental-petstore.bat to generate the python-experimental sample client on windows machines.
If merged, this PR will close out #1812

[spacether]

I am not sure why this test testCreateAndGetPetAsync failed on java okhttp-gson.
It doesn't look like my code update impacts it. When running the test locally the test passes:

Concurrency config is parallel='methods', perCoreThreadCount=true, threadCount=2, useUnlimitedThreads=false
Running org.openapitools.client.api.PetApiTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0 sec

EDIT: the below commit 0b5f299 fixed this issue and all tests now pass. Must have been a 👻

[wing328]

@spacether Is it correct to say sortParamsByRequiredFlag will no longer be supported after this PR is merged into master?

[wing328]

FROM
(req_arg1=None, req_arg2_with_default=None, optional_arg1=None):
TO:
(req_arg1, req_arg2_with_default=default_value, optional_arg1=None):

This will be a breaking change (without fallback). To get it merged into master, likely we will need an option.

[spacether]

@spacether Is it correct to say sortParamsByRequiredFlag will no longer be supported after this PR is merged into master?

Correct we will be hard coding in that required params come before optional ones and that flag will no longer be used.

[spacether]

FROM
(req_arg1=None, req_arg2_with_default=None, optional_arg1=None):
TO:
(req_arg1, req_arg2_with_default=default_value, optional_arg1=None):

This will be a breaking change (without fallback). To get it merged into master, likely we will need an option.

What does we will need an option mean?
would you prefer that I keep the arguments as named arguments rather than positional?

[wing328]

So required arguments without defaults are now positional
Required arguments with defaults are now named arguments which use the default value

I think only optional parameters can have default values. Usually, we don't include the default value in the client as it should be done in the server side instead.

[spacether]

FROM
(req_arg1=None, req_arg2_with_default=None, optional_arg1=None):
TO:
(req_arg1, req_arg2_with_default=default_value, optional_arg1=None):

This will be a breaking change (without fallback). To get it merged into master, likely we will need an option.

What makes you think that this will be a breaking change?

I do not think that this is a breaking change for the below reasons.
So far, people have been instantiating these models and calling these api endpoints with keyword arguments.
That will continue to work because python 2 + 3 let you use:

def func(arg1, arg2=None):
  pass
# one can invoke it with
func('arg1_val', arg2='blah')
# OR
func(arg1='arg1_val', arg2='blah')
# so keyword argument invocation still works for a function which has position arguments
# you can set a position argument using a keyword argument in python2 + 3

But we are loading in values in using a default value, won't that be a breaking change?

No it will not, because we are only loafing in values for REQUIRED parameters which have an enum of length one defined.
The current code base will:

• assign None by default and fail if None is passed in because it is not in the list of valid enums
• accept us only passing in the allowed enum value

The new code will:

• assign the valid enum value by default
• accept us only passing in the allowed enum value

[spacether]

So required arguments without defaults are now positional
Required arguments with defaults are now named arguments which use the default value

I think only optional parameters can have default values. Usually, we don't include the default value in the client as it should be done in the server side instead.

Let me try to describe this more clearly. Assigning default values in our function signatures is a way of passing in data. This is conceptually different than the swagger concept of a defualt value.

There are 4 swagger spec parameter use cases here:

• Case 1, REQUIRED parameter with an enum of length 1
  • This is valid
  • This PR assigns the enum value as the default value in the function signature because we know that this value is required and we know the value it must be (because there is only one option). The value is assigned using a keyword argument.
• Case 2, OPTIONAL parameter with an enum of length 1
  • This is valid
  • The current code base this PR do not assign a default value to this parameter in the function signature
• Case 3, REQUIRED parameter with default value
  • This is invalid per the swagger spec because default values are server side, only OPTIONAL parameters should have defaults. Per swagger.io Use the default keyword in the parameter schema to specify the default value for an optional parameter. The default value is the one that the server uses if the client does not supply the parameter value in the request.
• Case 4, OPTIONAL parameter with default value
  • This is valid
  • The current code base this PR do not assign a default value to this parameter in the function signature

[wing328]

Case 1, REQUIRED parameter with an enum of length 1

Is that some sort of default header that must be included in every requests?

[wing328]

Case 1, REQUIRED parameter with an enum of length 1

Is that some sort of default header that must be included in every request?

[wing328]

What makes you think that this will be a breaking change?

Thanks for the detailed explanation.

[spacether]

Case 1, REQUIRED parameter with an enum of length 1

Is that some sort of default header that must be included in every request?

My use case is headers with set values. But it could be any field where the value must be held constant and the filed is required.

[wing328]

Thanks for bringing up this particular use case about the required parameter (enum) with exactly 1 allowable value.

Technically it's allowed to have such parameter.

But I want to step back to understand why there's such a need to include the parameter (required, enum with 1 possible value) in the request. If the parameter (input from the client's request) is expected to be included in every single request, what value (or additional information) does it provide to the backend as the backend expects the same thing in every request?

I want to make the client as simple, easy-to-use as possible and having a required parameter with one possible value does not seem to be a good idea to me.

[spacether]

Thanks for bringing up this particular use case about the required parameter (enum) with exactly 1 allowable value.

Technically it's allowed to have such parameter.

But I want to step back to understand why there's such a need to include the parameter (required, enum with 1 possible value) in the request. If the parameter (input from the client's request) is expected to be included in every single request, what value (or additional information) does it provide to the backend as the backend expects the same thing in every request?

I want to make the client as simple, easy-to-use as possible and having a required parameter with one possible value does not seem to be a good idea to me.

This is a use case which routinely exists when headers are required.
So I think that it's less of is this a good idea versus do we have endpoints that require this now on servers and the answer is yes. The spec also allows any number of enum values. Enum of length one is a valid way of saying, there is only one possible value here.
What about it does not seem like a good idea to you?
Functions regularly have default values set, we are taking advantage of that to pass in data that we already know is required.
We already do this by setting the correct host domain for all endpoints, which is a single constant value.

[spacether]

Per a discussion with @wing328 we should merge this into a new generator, python-client-experimental
That generator will have its own version of the spec file, so updates that we make there will not update all of the other samples.
A separate PR will be written to make that new generator.

Note to self: see a similar-to spec file here: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/2_0/swift/petstore-with-fake-endpoints-models-for-testing.yaml

[spacether]

@wing328 this PR has been changed so it now updates the python-experimental generator

[spacether]

Gentle reminder that this PR needs a review. What's the status on it?
@wing328

[wing328]

@spacether thanks for the PR, which has been included in the 4.1.0 release: https://twitter.com/oas_generator/status/1160000504455319553