-
Notifications
You must be signed in to change notification settings - Fork 119
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
test: add type hints to test_model #1015
test: add type hints to test_model #1015
Conversation
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! Left a bunch of suggestions to simplify / clean a few things up.
Co-authored-by: Ben Hoyt <benhoyt@gmail.com>
Co-authored-by: Ben Hoyt <benhoyt@gmail.com>
Co-authored-by: Ben Hoyt <benhoyt@gmail.com>
Co-authored-by: Ben Hoyt <benhoyt@gmail.com>
Co-authored-by: Ben Hoyt <benhoyt@gmail.com>
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.
Thanks for the updates! I won't merge this until you make a final decision on those last outstanding comments -- your call as to whether to use typing.Any
or whatever there, or keep the annotations.
Required
that the other fields are not meant to be required, so make that the case (this also appears to match the code).Partially addresses #1007