-
Notifications
You must be signed in to change notification settings - Fork 78
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 Generic dataclasses #259
Open
mvanderlee
wants to merge
20
commits into
lovasoa:master
Choose a base branch
from
mvanderlee:generic-dataclasses
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
bc46a23
support generic dataclasses
onursatici db32163
support nested generic dataclasses
onursatici 8b82276
add test for repeated fields, fix __name__ attr for py<3.10
onursatici f2734cc
support py3.6
onursatici 9a5dc09
Split generic tests into it's own file.
mvanderlee 8443336
Add support for deep generics with swapped TypeVars.
mvanderlee 8a0f837
Fix tests after rebase
mvanderlee f484596
Remove unnecessary whitespace
mvanderlee 80dab91
fix call correct _field_for_schema function
mvanderlee 4531c35
Break generic functions out into it's own file and add support for an…
mvanderlee dd34efc
Remove support for callable annotations
mvanderlee 7ac088d
Remove support for annotated partials
mvanderlee b3362ba
Fix import style and some docstrings, and reuse is_generic_alias inst…
mvanderlee db95e64
Rename function to be more descriptive
mvanderlee a494984
Improved doc string
mvanderlee 78fcd4a
Clean up
mvanderlee 8797b2b
Rename our `is_generic_type` and only utilize where absolutely necessary
mvanderlee 4ef0bdf
Remove the need to call `get_type_hints`
mvanderlee d09312b
fix ensure we pass recursive_guard as kwarg
mvanderlee 3b5783a
Clean up unnessary if statements and redundant function call
mvanderlee File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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.
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.
_check_decorate_type
requires only thatisinstance(clazz, type)
.Do we want to require that clazz is a dataclass (
isinstance(clazz, type) and dataclasses.is_dataclass(clazz)
) here?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.
I don't think so because we already have a big warning when a class is not a dataclass in
_internal_class_schema
So non-dataclasses are allowed, but not supported
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.
I've just run a simple test.
You are correct in that decorating non-data class classes with
add_schema
works — at least in simple cases. (That doesn't necessarily mean that we should allow it.)It appears, however, that, as things stand in this PR, no big warning is emitted in that case. Further investigation reveals that
get_resolved_dataclass_fields
"just works" (with no warnings emitted) even if its argument is not a dataclass. (I have some suspicion that perhaps fields from the classes__mro__
are not properly handled in that case, but I haven't looked close enough to say for sure.)In any case, I think that either:
add_schema
decorator on non-dataclasses. (Why allow it if it's unsupported/untested?)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.
I've had to do some digging.
class_schema(NonDataclass)
just fine. It only shows the warning if one of it's fields is not a dataclass. i.e.: Nested non dataclasses.I don't disagree with removing support for non-dataclasses, but don't see why that should be part of this PR.