-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
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
| 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.) |
WillAyd
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.
lgtm feel free to merge @simonjayhawkins
No description provided.