Make instanceof schema work with arbitrary types #5947
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
please review |
adriangb
approved these changes
May 31, 2023
|
Too late! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspired by #5581 (reply in thread).
One of the original motivations (and I'm pretty sure requested in an issue I closed because I thought it was addressed..) of
InstanceOfwas to be able to remove the need forarbitrary_types_allowed=Truein the config by using this annotation, and only needing to use it on the actual arbitrary types. However as @adriangb noted, this doesn't currently work because it tries to generate the schema for loading from JSON.This change allows that to fail, in which case a standard is_instance_schema is used. This keeps some of the functionality behaving nicely (namely,
InstanceOf[Sequence[str]]remains compatible with JSON loading, etc.), but resolves one of the actual reasons thisInstanceOftype was introduced.I've also added it to the exports; I'm assuming its lack of inclusion was an oversight, but if we purposely weren't exporting it for the sake of not making it a "public API", then I guess we can remove it again, but I think these types merit inclusion in the public API.
Selected Reviewer: @adriangb