-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add JsonValue
type
#7998
Add JsonValue
type
#7998
Conversation
30de9c9
to
27a4d88
Compare
Deploying with Cloudflare Pages
|
I'll note that |
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.
LGTM, does this need more documentation?
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.
Nice work @dmontagu! Thanks for the detailed context in the description - the horrible error message was helpful in understanding why you implemented this in the way you did 😎.
# Conflicts: # tests/test_types.py
I'm trying to understand the context since this seems separate from |
@alexmojaki, looks like I missed an indent on the docs I added, I'll fix that now. |
Adds a
JsonValue
types with better behavior than the "naive" implementation.In particular, the most naive implementation:
actually leads to a recursion error:
While this behavior could perhaps be improved, this is currently expected since there is no ability to know the name of the JsonValue variable at runtime with this approach to defining the type alias.
With the next most naive implementation:
There are two issues:
To expand on this second bullet, let me demonstrate why this is a problem:
That beast of an error message comes from just trying to validate:
{'a': {'b': ...}}
!I was able to address the first bullet by adding an annotation that modifies the core schema to use an any_schema when validating from JSON, so there is no unnecessary overhead.
I was able to address the second bullet through the use of a tagged union with a callable discriminator, where the callable uses the type as the discriminator. With the implementation in this PR, the error you get is just: