-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TYP/CLN: Optional[Hashable] -> pandas._typing.Label #32371
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
TYP/CLN: Optional[Hashable] -> pandas._typing.Label #32371
Conversation
LGTM |
The only consideration point is that Do you see any value in trying to maintain that distinction? |
can you be specific on the offending changes. where Label is used for an index name, I don't see that as being different from an row/column name. |
@WillAyd any specific objections? i dont see any problem here @simonjayhawkins needs rebase |
pandas/core/generic.py
Outdated
@@ -200,7 +199,7 @@ def __init__( | |||
self, | |||
data: BlockManager, | |||
copy: bool = False, | |||
attrs: Optional[Mapping[Optional[Hashable], Any]] = None, | |||
attrs: Optional[Mapping[Label, Any]] = None, |
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 think this may be one case @WillAyd was thinking of. attrs
needn't have any any relation with the columns of the DataFrame, so using Label
may be confusing.
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.
Yea this is a great example (thanks @TomAugspurger for following up for me!)
@simonjayhawkins what do you think about reverting the |
will do. we have flakey ci atm but will give it a go. (we have a fix for ci pending in #32449 but if we merge @SaturnFromTitan doesn't get credit (see #32457) or could just revert #32281 for now instead.) |
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.
lgtm feel free to merge @simonjayhawkins
No description provided.