Skip to content

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

Merged
merged 5 commits into from
Mar 6, 2020

Conversation

simonjayhawkins
Copy link
Member

No description provided.

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Feb 29, 2020
@jbrockmendel
Copy link
Member

LGTM

@WillAyd
Copy link
Member

WillAyd commented Feb 29, 2020

The only consideration point is that Label at least from a reader point of view was supposed to signal something that would actually be used as a Label for a row / column. Some of the uses here blur that distinction and simply leverage it because it can be used as the key for a mapping

Do you see any value in trying to maintain that distinction?

@simonjayhawkins
Copy link
Member Author

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.

@jbrockmendel
Copy link
Member

@WillAyd any specific objections? i dont see any problem here

@simonjayhawkins needs rebase

@@ -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,
Copy link
Contributor

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.

Copy link
Member

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!)

@jbrockmendel
Copy link
Member

@simonjayhawkins what do you think about reverting the attrs annotation and calling this a win?

@simonjayhawkins
Copy link
Member Author

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.)

Copy link
Member

@WillAyd WillAyd left a 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

@simonjayhawkins simonjayhawkins merged commit 8914c01 into pandas-dev:master Mar 6, 2020
@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Mar 6, 2020
@simonjayhawkins simonjayhawkins deleted the typing-label branch March 6, 2020 07:13
SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants