-
Notifications
You must be signed in to change notification settings - Fork 30
feat: support additional string format types, preventing failure on unknown types #90
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
base: main
Are you sure you want to change the base?
Conversation
elif schema.type == "string" and schema.schema_format == "decimal" and common.get_use_orjson(): | ||
converted_type = pre_type + "Decimal" + post_type | ||
import_types = ["from decimal import Decimal"] | ||
elif schema.type == "string": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also captures unknown "schema_format" that would earlier result in "unknown type string" errors.
Hi @MarcoMuellner , I followed the contributing guide and ran nox, but there were a huge number of linting errors that seem to come from prior changes, unrelated to my PR. Let me know if it's ok for me to allow those to continue to fail. |
), | ||
), | ||
( | ||
Schema(type=DataType.STRING, schema_format="email"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that "email" isn't specifically handled by the generator, this is just the test to make sure unexpected formats don't break.
Sorry, it looks like orjson doesn't support decimal after some review. I will need to remove that from this pr. Some attempt was made to support it upstream, but was never merged in ijl/orjson#399 |
With pydantic v2 model_dump_json() works perfectly fine, as an alternative to orjson. Shall I add in support for this ? |
@MarcoMuellner after more extensive testing I've found that certain types not supported by orjson can still be supported by pydantic v2. I believe that there are situations where a user may favour speed, in which case pydantic v2 and orjson will typically still be used together, meaning the types need to cater to the lowest common denominator. Thus I've split the logic into code that handles pydantic v1/2, and orjson enabled/disabled, with test fixtures to handle each case. Notable cases are Decimal, which is only supported by pydantic v2 natively, but not orjson, "date" format that is supported by both. Unknow or unsupported formats now fail back to "str" instead of erroring. |
I'm going to mark this as draft to do some more deep end to end testing and review. Will maker as ready once I am satisfied I've done the necessary work to prove it's correct. :) |
Hey there! Thank you for contributing! :) yeah ignore that - i need to fix that at some point so that this doesn't happen anymore. Do you want me to take a look yet, or do you want to still finish the PR? I personally find Speed not as important, rather than features. If pydantic gives you better results, then i am all for that and we'll remove another dependency in orjson :) |
I've already found one regression, model_dump_json() should be model_dump("json"), since httpx etc internally serializes to string :) |
I have a few more updates (changes will preceed tests), including support for polmorphic classes that can be either pure or partial compositions of parent classes. E.g. description: A connection with entity, plan and status details used for responses
allOf:
- $ref: EntityBase.yml
- $ref: Connection.yml
- type: object
required:
- status
- plan
- address_details
properties:
first_active_date:
type: string
format: date-time
description: The date that this connection was ever first active. If the plan changes then active_date might change but not first_active_date. will map to a class with "EntityBase" and "Connection" as parent classes. |
I should check in on my progress. Our project with this fork has been going along well, and we have been making a list of things that we still want to address in our merge:
I need to spend more time diving into the implementation of field types in this repo, since I still don't have a strong enough understanding to be confident in correct usage. I've mostly been focusing on model-level scaffolding up until now. :) |
I've also added a new option.
I'll probably need to rename this PR as it's now a little outside of the scope of the original intent. :) |
Closes #89