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

chore: fix pandas-stubs issues #2008

Merged
merged 17 commits into from
Feb 17, 2025

Conversation

MarcoGorelli
Copy link
Member

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

  • Related issue #<issue number>
  • Closes #<issue number>

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@MarcoGorelli MarcoGorelli marked this pull request as ready for review February 13, 2025 17:53
@EdAbati
Copy link
Collaborator

EdAbati commented Feb 14, 2025

This looks great! Tiny question:
Can we remove the "pandas.*" from the mypy ignores or we are not there yet?

@MarcoGorelli
Copy link
Member Author

thanks - yup that works!

@dangotbanned
Copy link
Member

dangotbanned commented Feb 15, 2025

@MarcoGorelli do you wanna steal all the related fixes I did in the tests from #2007?

Once pyarrow-stubs is added, these ones will start failing (even after your solutions for pandas-stubs)

Screenshot

image

dangotbanned added a commit that referenced this pull request Feb 15, 2025
@MarcoGorelli the ignore(s) will show up for `mypy` after #2008 (I assume)
#2007 (review)
@@ -26,10 +26,10 @@


def n_unique() -> dd.Aggregation:
def chunk(s: pd.core.groupby.generic.SeriesGroupBy) -> int:
def chunk(s: pd.core.groupby.generic.SeriesGroupBy) -> pd.Series[Any]:
Copy link
Member

@dangotbanned dangotbanned Feb 17, 2025

Choose a reason for hiding this comment

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

I'm getting quite a lot of warnings for pd.core from pyright

Screenshot

image

I think I resolved this before in altair by finding a more public import path

Need to see if we can drop the `suppress` on older versions
narwhals-dev#2008 (comment)
Comment on lines +515 to +519
sentinel = object()
if (
isinstance(dtype, pd.api.extensions.ExtensionDtype)
and getattr(dtype, "base", sentinel) is None
):
Copy link
Member

@dangotbanned dangotbanned Feb 17, 2025

Choose a reason for hiding this comment

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

Related to avoiding pd.core (#2008 (comment))

Available since 0.23.0

Pretty sure we don't need with suppress(AttributeError): with this?
@MarcoGorelli

@dangotbanned
Copy link
Member

dangotbanned commented Feb 17, 2025

I have a feeling all of these tests were not being type checked - because they had a pandas import

Like I was already seeing this with pyright, but suddenly mypy also complains

Screenshot

image

Resolved all 40+ of the new errors in (#2008 (commits))

Copy link
Member

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

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

@MarcoGorelli I'm happy to merge like this and follow up with a deeper look into reducing pd.core (https://github.com/narwhals-dev/narwhals/pull/2008/files#r1958289318)

I don't know why mypy isn't warning about this, but seems to be the next target in making pyright happier

@MarcoGorelli
Copy link
Member Author

amazing, thanks a lot @dangotbanned !

@MarcoGorelli MarcoGorelli merged commit d2d55da into narwhals-dev:main Feb 17, 2025
25 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants