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

[REQ] Python Improved Model Template #4807

Closed
MikeMcGarry opened this issue Dec 16, 2019 · 9 comments
Closed

[REQ] Python Improved Model Template #4807

MikeMcGarry opened this issue Dec 16, 2019 · 9 comments

Comments

@MikeMcGarry
Copy link

MikeMcGarry commented Dec 16, 2019

Is your feature request related to a problem? Please describe.

When using an IDE e.g. PyCharm with SDKs generated using the openapi-generator in Python. The docstring in the "init" method for any models is next to useless. All it says by default is

"""{modelName} - a model defined in OpenAPI". """

There is therefore little value in the type hints and docstring when developing using an IDE.

In addition there is no way to easily tell the difference between the required and optional parameters outside of looking at the setters and if they have a None check.

Describe the solution you'd like

It would be great to have a docstring with the parameter descriptions and types which helps with using the SDK.

For example (a snippet from our LUSID Python SDK):

"""
       ConfigurationRecipeSnippet - a model defined in OpenAPI

       :param aggregation_options: 
       :type aggregation_options: lusid.AggregationOptions
       :param model_rules:  The set of model rules that are available. There may be multiple rules for Vendors, but only one per model-instrument pair.  Which of these preference sets is used depends upon the model choice selection if specified, or failing that the global default model specification  in the options.
       :type model_rules: list[lusid.VendorModelRule]
       :param pricing_options: 
       :type pricing_options: lusid.PricingOptions
       :param market_rules:  The set of rules that define how to resolve particular use cases. These can be relatively general or specific in nature.  Nominally any number are possible and will be processed in order where applicable. However, there is evidently a potential  for increased computational cost where many rules must be applied to resolve data. Ensuring that portfolios are structured in  such a way as to reduce the number of rules required is therefore sensible.
       :type market_rules: list[lusid.MarketDataKeyRule]
       :param market_options: 
       :type market_options: lusid.MarketOptions
       :param recipe: 
       :type recipe: lusid.ConfigurationRecipe

       """  # noqa: E501

Would come from the following template:

        """
        {{classname}} - a model defined in OpenAPI

        {{#vars}}
        :param {{name}}: {{#description}} {{{description}}}{{/description}}{{#required}} (required){{/required}}{{#optional}}(optional{{#defaultValue}}, default to {{{.}}}{{/defaultValue}}){{/optional}}
        :type {{name}}: {{^complexType}}{{dataType}}{{/complexType}}{{#complexType}}{{^isContainer}}{{packageName}}.{{complexType}}{{/isContainer}}{{#isListContainer}}list[{{packageName}}.{{complexType}}]{{/isListContainer}}{{#isMapContainer}}dict[str, {{packageName}}.{{complexType}}]{{/isMapContainer}}{{/complexType}}
        {{/vars}}

        """  # noqa: E501

It would also be great to have a required_map which notes down which attributes are required and optional.

E.g:

    required_map = {
        'aggregation_options': 'optional',
        'model_rules': 'optional',
        'pricing_options': 'optional',
        'market_rules': 'optional',
        'market_options': 'optional',
        'recipe': 'optional'
    }

Which would come from the following in the template:

    required_map = {
{{#vars}}
        '{{name}}': '{{^required}}optional{{/required}}{{#required}}required{{/required}}'{{#hasMore}},{{/hasMore}}
{{/vars}}
    }

Describe alternatives you've considered

We were able to solve both these issues with a custom template. I think the custom template we created is generic enough to be used as the default model template and want to see if we should merge it in.

@spacether
Copy link
Contributor

spacether commented Dec 19, 2019

@MikeMcGarry the python-experimental model classes have description of their class variables and the arg required and optional kwarg inputs for the model. See an example here:
https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/python-experimental/petstore_api/models/pet.py#L114

Does that meet your needs?

If so, we may want to also use that code in the python client.

Detailed python-experimental info

In python-experimental:

  • Model required inputs are in the positional args
  • Model optional inputs are in the kwargs
  • Operation required inputs are in positional args
  • Operation optional inputs are in kwargs

Operation examples:

Note: we also standardized how we access allowed_values in models and endpoints in that generator.

@spacether
Copy link
Contributor

Related: #4600

@phemmer
Copy link

phemmer commented Jan 9, 2021

After trying to generate a python model for a new project using the 4.3.1 generator, and finding it to be less than good, I tried using the 5.0.0 generator only to find it's drastically worse.

With the 4.3.1 generator __init__ at least had arguments for the optional parameters. The 5.0.0 doesn't, instead relying on **kwargs.
As another major regression, in 4.3.1 all the properties were defined on the class (using accessor methods), allowing an IDE to discover them. In 5.0.0 they are no longer there.

So basically in 4.3.1, the IDE at least had some idea that these arguments & properties existed, even if it didn't know the type. Now it has no clue.
While I don't understand the reason for all this metaprogramming, it's making it impossible to work with static code analysis.

If we're going to continue with this metaprogramming design, can we at least generate stub files to address the issue?

@spacether
Copy link
Contributor

spacether commented Jan 9, 2021

We welcome any prs which will add type definition to the signatures of the methods and functions in the python generators and alow for static analysis. We also welcome any PRs which add stubs or attribute accessors which help IDEs.
Types are described in the docstrings.

Also the Python generator is using strict typing for model instance inputs and endpoint inputs and responses so if a type is wrong an error will be thrown.

Optional arguments were moved into kwargs because they can be nullable. The old way would look like:
endpoint_fn(required_arg1, optional_arg2=None)
When sending data to the endpoint we have the following optional_arg2 use cases:

  1. optional_arg2 is omitted
  2. optional_arg2 is null (None in python) and needs to be sent to the server
  3. optional_arg2 has another value and needs to be sent to the server

With the original function signature, in the endpoint function client side we had no way to tell use case 1 + 2 apart because the code was using a value of None to check if a value had been assigned.

@spacether
Copy link
Contributor

@phemmer what IDE are you using?

@phemmer
Copy link

phemmer commented Feb 4, 2021

IntelliJ IDEA/PyCharm

@johnthagen
Copy link
Contributor

@spacether

Related: #4600

Note that the limitation of the number of arguments no longer exists in Python 3.7+

And with Python 3.6 going end of life in less than 2 months, this concern may be less of a concern: https://endoflife.date/python

@spacether
Copy link
Contributor

spacether commented Jan 5, 2022

Heads up. We now have a new generator, python-experimental which has type hints for instantiating models and for endpoint parameters. It was added in: #8325

@spacether
Copy link
Contributor

spacether commented Jan 11, 2022

Closing this issue because the python-experimental generator includes the requested model type hints in the model __new__ signature. These type hints show up in pycharm. One can see it working in the Pet model here:
https://github.com/OpenAPITools/openapi-generator/blob/master/samples/openapi3/client/petstore/python-experimental/petstore_api/model/pet.py#L137

    def __new__(
        cls,
        *args: typing.Union[dict, frozendict, ],
        name: name,
        photoUrls: photoUrls,
        id: typing.Union[id, Unset] = unset,
        category: typing.Union['Category', Unset] = unset,
        tags: typing.Union[tags, Unset] = unset,
        status: typing.Union[status, Unset] = unset,
        _instantiation_metadata: typing.Optional[InstantiationMetadata] = None,
        **kwargs: typing.Type[Schema],
    ):

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

No branches or pull requests

4 participants