-
Notifications
You must be signed in to change notification settings - Fork 12
Operations: Add type hints #268
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
Conversation
antirotor
left a comment
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.
Things to consider - using TypedDict to construct return values and how to deal with NOT_SET - perhaps changing NOT_SET and similar to class and type hint that?
| folder_id: str, | ||
| name: Optional[str] = None, | ||
| folder_type: Optional[str] = None, | ||
| parent_id: Optional[str] = NOT_SET, |
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.
This won't work as NOT_SET is object, but the attribute is typed as Optional[str].
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.
And what would be the solution? How to type-hint placeholder objects?
The type-hint for "developer using the code" is Optional[str] if he passes the value. I really don't want to block this PR just because it is not mypy compatible, the release of python api should have been done 2 weeks ago.
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.
Not functional change, should not block the merge anyway. Would be nice to find how to solve that though.
Changelog Description
Fill type hints in operations code.
Testing notes: