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] add support for decimals in the python generator #13324

Closed
rizwansaeed opened this issue Aug 31, 2022 · 10 comments
Closed

[REQ] add support for decimals in the python generator #13324

rizwansaeed opened this issue Aug 31, 2022 · 10 comments

Comments

@rizwansaeed
Copy link
Contributor

Describe the solution you'd like

Add support to the python generator for the following:

units:
  type: string
  format: decimal

which would generate a field of type decimal.Decimal that is de/serialized across the wire as a string.

I am aware that support has been added to the python-experimental as part of #11282 but we are looking for support in the python generator.

Additional context

We'd be happy to raise a PR but are looking for a steer on what the required changes would be and how long it might take to implement.

Thanks

@spacether
Copy link
Contributor

spacether commented Sep 1, 2022

One could maybe add it by changing the mapping for its type to Decimal, but how would it interact with over validation where the type could be plain string with no formatting?
Python-experimental handles those use cases much more elegantly. Just curious, why do you prefer the python client over the python-experimental one? It has the feature that you are looking for, Have you tried it?

@rizwansaeed
Copy link
Contributor Author

I was thinking to essentially do as you say such that the openapi_types mapping in the generated model class maps to a Decimal. So type: string maps to str and type: string, format: decimal maps to Decimal.

For the use case you describe where no formatting is given, it would be treated as a string.

If that sounds like a reasonable approach, we're looking for some guidance as to what code changes would need to be made.

Wrt python-experimental, we use the generated SDK in a production environment and so the "experimental" tag led us to believe it wasn't quite finished. Is that that the case or are there still more features that need adding? We'd be open to helping close any gaps if that is the case.

Thanks

@spacether
Copy link
Contributor

spacether commented Sep 1, 2022

Experimental refers to the fact that it has been out for a shorter period of time and that we allow breaking changes to be made to that generator. If you version pin you should be fine.
Others are using it in production.
It has many more features than the python generator including

  • decimal support
  • type hints everywhere
  • multiple request and response content type handling
  • more robust openapi unit testing

Please give it a try and let me know if it meets your needs. It is not missing any major features.

@rizwansaeed
Copy link
Contributor Author

thanks, will give a try and revert back 👍

@spacether
Copy link
Contributor

Glad to hear it. If you use the latest master branch there are a ton of type hint improvements and it works great in pycharm.

These are the latest improvements:
#13325 #13323 #13314 #13309 #13299 #13271

@rizwansaeed
Copy link
Contributor Author

Hi Justin,

I've had a try with the python-experimental generator. There seems to be a number of breaking changes between the it and the python one (which are reasonable and I suppose expected), but this does mean that adoption would take us longer to complete.

I did manage to almost get a couple of calls working but we've hit a blocking issue:

NotImplementedError: Serialization has not yet been implemented for application/json-patch+json

So for now at least we are probably going to need to try to add the feature into the existing python one. Would you be able to provide some guidance as where in the generator code we would need to make the changes?

Thanks

@spacether
Copy link
Contributor

spacether commented Sep 5, 2022

Hi there, I just fixed your blocking issue in this PR which has been merged into master branch.
This test verifies that json patch serialization and sending is working and it passed in CI here.
So my suggestion is to use python-experimental because:

  • it now has the features that you need
  • updating your code with the needed changes migrating python to python-experimental will probably take less time than writing a PR adding decimal to the python generator
  • There is a migration guide that points out the changes that you need to make
  • the python generator needs all value stored for a property in multiple validated schemas to be the same. if there are two allOf schemas that constrain the value and one defines a decimal as type: string and another defines it as type: string format: decimal I do not think that it will work in the python generator. That definitely WILL work in the python-experimental generator.

If you do want to move forward with a PR in the python generator then these are some places that will need to change (from looking at how datetime is done):

@rizwansaeed
Copy link
Contributor Author

Thanks for fixing that so quickly, I'll take a look at the experimental and python changes

@spacether
Copy link
Contributor

my mistake re-opening this

@spacether spacether reopened this Sep 16, 2022
@spacether
Copy link
Contributor

Closing this because the current python generator in v6.2.0 and onward supports decimals

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

2 participants