Skip to content

chore(typing): enable reportIncompatibleMethodOverride in pyright#3096

Merged
MarcoGorelli merged 7 commits intonarwhals-dev:mainfrom
MarcoGorelli:reportincompatiblemethodoverride
Sep 7, 2025
Merged

chore(typing): enable reportIncompatibleMethodOverride in pyright#3096
MarcoGorelli merged 7 commits intonarwhals-dev:mainfrom
MarcoGorelli:reportincompatiblemethodoverride

Conversation

@MarcoGorelli
Copy link
Member

I was trying out Pyrefly and it was flagging some of these. I initially thought it was a false postive but then realised pyright strict flags them too

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 September 6, 2025 10:55
@dangotbanned
Copy link
Member

Hey Marco!

I'm 90% sure I'm on board, but noticed a couple of things on the diff I wanna check over later - if that's okay?

@dangotbanned
Copy link
Member

Oooh also it reminded me of trying out typeCheckingMode = "strict" a couple months back

reportUnusedExpression = "none"    # handled by (https://docs.astral.sh/ruff/rules/unused-variable/)
+ # strict
+ reportUnusedVariable = "none"      # handled by (https://docs.astral.sh/ruff/rules/unused-variable/)
+ reportUnusedClass = "none"
+ reportUnusedFunction = "none"
+ reportPrivateUsage = "none"
+ reportUnknownVariableType = "none"
+ reportUnknownArgumentType = "none"
+ reportUnknownMemberType = "none"
+ reportUnknownLambdaType = "none"
+ reportMissingTypeStubs = "none"
+ typeCheckingMode = "strict" # strict = 3545 errors

I'd gotten the branch down to 81 errors (June 26, 2025), and reportIncompatibleMethodOverride I thought we should keep as well 🙂

@dangotbanned dangotbanned self-requested a review September 6, 2025 11:17
Comment on lines 1801 to 1803
def join(
self,
other: Self,
other: BaseFrame[DataFrameT],
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really liking how this one leaks out into the docs

Show docs

image

But more importantly it breaks the existing warnings here:

import polars as pl

import narwhals as nw

data = {"a": ["A", "B", "A"], "b": [1, 2, 3]}

df = nw.from_native(pl.DataFrame(data))

okay = df.join(df, on="a")

# Previously reported an error
# Argument of type "LazyFrame[Any]" cannot be assigned to parameter "other" of type "narwhals.dataframe.DataFrame[polars.dataframe.frame.DataFrame]" in function "join"
#  "LazyFrame[Any]" is not assignable to "narwhals.dataframe.DataFrame[polars.dataframe.frame.DataFrame]"

bad = df.join(df.lazy(), on="a")

LazyFrame[Any] can be assigned to BaseFrame[DataFrameT]

Copy link
Member

Choose a reason for hiding this comment

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

I probably wouldn't have picked this up were it not for this recent PR 😄

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion

Just for this one, since it solves the two issues in https://github.com/narwhals-dev/narwhals/pull/3096/files#r2328197883

PolarsBaseFrame is fine as it's internal and the subclasses fill the constrained TypeVar anyway 🥳

diff --git a/narwhals/dataframe.py b/narwhals/dataframe.py
index 9cc5382c7..5b2752e02 100644
--- a/narwhals/dataframe.py
+++ b/narwhals/dataframe.py
@@ -91,6 +91,7 @@ if TYPE_CHECKING:
     )
 
     PS = ParamSpec("PS")
+    Incomplete: TypeAlias = Any
 
 _FrameT = TypeVar("_FrameT", bound="IntoFrame")
 LazyFrameT = TypeVar("LazyFrameT", bound="IntoLazyFrame")
@@ -284,7 +285,7 @@ class BaseFrame(Generic[_FrameT]):
 
     def join(
         self,
-        other: BaseFrame[_FrameT],
+        other: Incomplete,
         on: str | list[str] | None,
         how: JoinStrategy,
         *,
@@ -335,7 +336,7 @@ class BaseFrame(Generic[_FrameT]):
 
     def join_asof(
         self,
-        other: BaseFrame[_FrameT],
+        other: Incomplete,
         *,
         left_on: str | None,
         right_on: str | None,
@@ -1800,7 +1801,7 @@ class DataFrame(BaseFrame[DataFrameT]):
 
     def join(
         self,
-        other: BaseFrame[DataFrameT],
+        other: Self,
         on: str | list[str] | None = None,
         how: JoinStrategy = "inner",
         *,
@@ -1846,7 +1847,7 @@ class DataFrame(BaseFrame[DataFrameT]):
 
     def join_asof(
         self,
-        other: BaseFrame[DataFrameT],
+        other: Self,
         *,
         left_on: str | None = None,
         right_on: str | None = None,
@@ -3052,7 +3053,7 @@ class LazyFrame(BaseFrame[LazyFrameT]):
 
     def join(
         self,
-        other: BaseFrame[LazyFrameT],
+        other: Self,
         on: str | list[str] | None = None,
         how: JoinStrategy = "inner",
         *,
@@ -3107,7 +3108,7 @@ class LazyFrame(BaseFrame[LazyFrameT]):
 
     def join_asof(
         self,
-        other: BaseFrame[LazyFrameT],
+        other: Self,
         *,
         left_on: str | None = None,
         right_on: str | None = None,

Copy link
Member Author

Choose a reason for hiding this comment

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

seems reasonable

Copy link
Member

Choose a reason for hiding this comment

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

Resolved in (be reasonable) 😉

@dangotbanned
Copy link
Member

dangotbanned commented Sep 7, 2025

Hey Marco!

I'm 90% sure I'm on board, but noticed a couple of things on the diff I wanna check over later - if that's okay?

As an update to this, besides what I mentioned in (#3096 (comment)):

  • Looks good
    • keyword-only marker in {Expr,Series}.{std,var}, SparkLikeLazyFrame.join
    • Parameter name mismatch in IbisExprDateTimeNamespace.offset_by
  • Probably correct
    • stable.v*.DataFrame.to_dict
  • Would like some more time to experiment

Now I do think we should have this rule enabled.
I mainly just want to be sure the way we're choosing to fix/silence the warnings are:

  1. Not causing some issue elsewhere (e.g. like chore(typing): enable reportIncompatibleMethodOverride in pyright #3096 (comment))
  2. Will be what any other contributor can look to for how they should solve a [reportIncompatibleMethodOverride] in the future 🙂

@dangotbanned dangotbanned changed the title chore: enable reportIncompatibleMethodOverride in pyright chore(typing): enable reportIncompatibleMethodOverride in pyright Sep 7, 2025
Comment on lines 912 to 915

ewm_mean: not_implemented = not_implemented() # pyright: ignore[reportIncompatibleMethodOverride]
map_batches: not_implemented = not_implemented() # pyright: ignore[reportIncompatibleMethodOverride]
replace_strict: not_implemented = not_implemented() # pyright: ignore[reportIncompatibleMethodOverride]
ewm_mean = not_implemented() # type: ignore[misc]
map_batches = not_implemented() # type: ignore[misc]
replace_strict = not_implemented() # type: ignore[misc]
Copy link
Member

Choose a reason for hiding this comment

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

Provided this doesn't blow up something else I think this is the least bad option for now 😅

  • pyright even on strict mode is fine with it
  • doing the "fix" that mypy wants, is what introduces the need to ignore reportIncompatibleMethodOverride
  • Typing them as Any - while shorter - does mask this issue
    • eventually I'd like to fix it
    • probably won't remember without all the ugliness 😄

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.

this narwhal loves you

@MarcoGorelli
Copy link
Member Author

thanks @dangotbanned !

@MarcoGorelli MarcoGorelli merged commit aa7a7a7 into narwhals-dev:main Sep 7, 2025
32 of 33 checks passed
dangotbanned added a commit that referenced this pull request Sep 13, 2025
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.

2 participants