-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
CLN: Add/refine type hints to some functions in core.dtypes.cast #33286
Conversation
- maybe_cast_result obj is now "Series" - Added type hint and assert to maybe_cast_to_extension_array - Added generic type to _GroupBy and GroupBy in core.groupby.groupby - Updated missed "try_cast" references to "maybe_cast"
@@ -329,6 +333,8 @@ def maybe_cast_to_extension_array(cls, obj, dtype=None): | |||
ExtensionArray or obj | |||
""" | |||
assert isinstance(cls, type), f"must pass a type: {cls}" | |||
assertion_msg = f"must pass a subclass of ExtensionArray: {cls}" | |||
assert issubclass(cls, ABCExtensionArray), assertion_msg |
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.
Is the assert here still necessary, or does the type hint now make this redundant?
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.
@simonjayhawkins Would like your thoughts on this.
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.
the type hint is sufficient for the static checking (so that mypy doesn't report any errors). For the assert, it depends on the desired runtime behaviour.
see #32302 (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.
Thanks @rhshadrach lgtm pending green
thanks @rhshadrach very nice! |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Some cleanup resulting from PR #32894. In order to get SeriesGroupBy._transform_fast to realize self.obj is a Series, it seemed I needed to add a generic type to GroupBy and _GroupBy. I don't know if there is a better way to do this. Here, one cannot use FrameOrSeries as a Series may return a DataFrame and vice-versa, so I added _GroupByT.