Skip to content
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 #37715: Fix mypy errors in column.py #50164

Closed
wants to merge 6 commits into from

Conversation

bang128
Copy link
Contributor

@bang128 bang128 commented Dec 10, 2022

@@ -81,7 +81,7 @@ def __init__(self, column: pd.Series, allow_copy: bool = True) -> None:
self._col = column
self._allow_copy = allow_copy

def size(self) -> int: # type: ignore[override]
def size(self) -> int:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to add the @property decorator to get this to work with mypy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it in my first commit, but the tests failed, so I tried removing @Property

@WillAyd
Copy link
Member

WillAyd commented Dec 11, 2022

Huh strange - I guess that's why this was ignored before. That said, I think the property decorator is correct, at least according to the dataframe protocol

https://data-apis.org/dataframe-protocol/latest/API.html

So I think you can update the tests and add a whatsnew note describing this as a bug fix

@jorisvandenbossche

@jorisvandenbossche
Copy link
Member

That said, I think the property decorator is correct, at least according to the dataframe protocol

https://data-apis.org/dataframe-protocol/latest/API.html

Hmm, the documentation seems to be not up to date. This was originally a property, but changed to a method a while ago (data-apis/dataframe-api#74 (comment)), so the currently implementation here is actually correct.

@mroeschke
Copy link
Member

Thanks for the pull request, but I believe this was addressed and fixed in #50565 so closing. Happy to have your contributions on any other typing issues!

@mroeschke mroeschke closed this Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants