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

Allow dicts to have both patternProperties and additionalProperties #4642

Closed
wants to merge 1 commit into from

Conversation

jparise
Copy link

@jparise jparise commented Oct 22, 2022

Support for patternProperties was introduced in #332, but that logic unfortunately made patternProperties and additionalProperties mutually exclusive. JSON Schema supports their combination:

https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties

This prevented well-typed code generation for dictionaries that use regex-constrained string keys.

Support for `patternProperties` was introduced in pydantic#332, but that logic
unfortunately made `patternProperties` and `additionalProperties`
mutually exclusive. JSON Schema supports their combination:

https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties

This prevented well-typed code generation for dictionaries that use
regex-constrained string keys.
@jparise
Copy link
Author

jparise commented Oct 22, 2022

This patch is for the main branch (v2). The v1.10.x patch is #4641.

@samuelcolvin
Copy link
Member

All this logic will be rewritten for V2, so we can't accept this. #4516 will be merged pretty soon, then we can start working on schema for V2.

Possibly we could accept this as a bug fix for v1, please create a new PR we'll review.

@jparise
Copy link
Author

jparise commented Oct 22, 2022

All this logic will be rewritten for V2, so we can't accept this. #4516 will be merged pretty soon, then we can start working on schema for V2.

Understood. I opened this PR for completeness after I opened #4641 for v1.

Possibly we could accept this as a bug fix for v1, please create a new PR we'll review.

Cool. I'm primarily interested in fixing this in v1 (#4641) and then helping to guard against a regression in v2.

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

Successfully merging this pull request may close these issues.

None yet

2 participants