Skip to content

CLN: #1836 merge pyrefly_strict into pyrefly, make ty_all and pyrefly_all passing - #1867

Open
cmp0xff wants to merge 47 commits into
pandas-dev:mainfrom
cmp0xff:pyrefly-strict-ty-all
Open

CLN: #1836 merge pyrefly_strict into pyrefly, make ty_all and pyrefly_all passing#1867
cmp0xff wants to merge 47 commits into
pandas-dev:mainfrom
cmp0xff:pyrefly-strict-ty-all

Conversation

@cmp0xff

@cmp0xff cmp0xff commented Aug 2, 2026

Copy link
Copy Markdown
Contributor
  • Addresses TST: "strict" modes for mypy, pyrefly and ty #1836 (Replace xxxx with the Github issue number)
  • Tests added (Please use assert_type() to assert the type of any return value)
  • If I used AI to develop this pull request, I prompted it to follow AGENTS.md.

@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch 2 times, most recently from 2232058 to 461372e Compare August 2, 2026 23:10
@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch 7 times, most recently from 0122fa5 to 2770692 Compare August 5, 2026 21:26
loicdiridollou pushed a commit that referenced this pull request Aug 5, 2026
@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch 2 times, most recently from 497906a to c3e92f5 Compare August 5, 2026 22:27
loicdiridollou pushed a commit that referenced this pull request Aug 6, 2026
fix: categorical
@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch from c3e92f5 to bd902dc Compare August 6, 2026 07:32
@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch from 642ad36 to 24d0eff Compare August 6, 2026 09:30
loicdiridollou pushed a commit that referenced this pull request Aug 6, 2026
….pyi` (#1871)

* `@override` in `Timestamp`

* one more override

* fix run
@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch 2 times, most recently from d661339 to 51fbbbc Compare August 6, 2026 15:18
@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch 3 times, most recently from 4279c42 to 040bab0 Compare August 13, 2026 21:59
@cmp0xff cmp0xff mentioned this pull request Aug 14, 2026
3 tasks
@cmp0xff
cmp0xff force-pushed the pyrefly-strict-ty-all branch from f2723c4 to 299ad19 Compare August 14, 2026 11:44
@cmp0xff cmp0xff changed the title CLN: #1836 merge pyrefly_strict into pyrefly, ty_all into ty CLN: #1836 merge pyrefly_strict into pyrefly, make ty_all passing Aug 14, 2026
@cmp0xff cmp0xff changed the title CLN: #1836 merge pyrefly_strict into pyrefly, make ty_all passing CLN: #1836 merge pyrefly_strict into pyrefly, make ty_all and pyrefly_all passing Aug 14, 2026
@cmp0xff
cmp0xff requested a review from Dr-Irv August 18, 2026 13:49

@Dr-Irv Dr-Irv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

did go through the tests, so those are reviewed.

There is one open comment from previous review - others were all resolved.

Comment thread pandas-stubs/core/arrays/interval.pyi Outdated
@override
def nbytes(self) -> int: ...
@property
def size(self) -> int: ...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you can remove size here

Comment thread pandas-stubs/core/indexes/multi.pyi Outdated
Comment on lines +218 to +220
@override
# pyrefly: ignore[bad-override]
def isin(self, values: Iterable[Any], level: Level) -> np_1darray_bool: ...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, but the code defines isin allowing None as the argument for level here:
https://github.com/pandas-dev/pandas/blob/4e1b1f77dbc700e505c808e8b6bc164abce87955/pandas/core/indexes/multi.py#L4884

So I think we can delete this override and the tests should still work.

Comment on lines +225 to 226
@override
def set_names(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK - the implementation is just in Index, but tests for MultiIndex being required if Mapping is the argument

Comment thread tests/frame/test_frame.py Outdated

def my_named_func_1(df: pd.DataFrame) -> pd.Series[str]:
return df["a"]
return cast("pd.Series[str]", df["a"])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we add a comment here why we're OK with the cast in the test?


def sum_mean(x: pd.DataFrame) -> float:
return x.sum().mean()
return cast("pd.Series[float] | pd.Series[int]", x.sum()).mean()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a fan of the cast here. Why should a user be required to do the cast to get this to work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ty reveals x.sum() as Series[Any], and Series[Any]().mean() as Unknown. What do you think? We can make Series[Any]().mean() giving Any.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

At least for pyright, x.sum().mean() is revealed as float there. So check the other type checkers, as this might be a bug in ty

Comment thread tests/test_groupby.py
return Series(val)

def s2scalar(val: Series) -> float:
def s2scalar(val: Series[int] | Series[float]) -> float:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same comment as above

Comment thread tests/test_groupby.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Multiple places here (I didn't comment on all of them), but we want functions declared as:

def foo(val: Series) -> float:

to be acceptable usage. We can't force people to use the generic declarations like Series[int] and Series[float].

Is there a way to keep the tests as they were?

Comment thread tests/test_pandas.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same comment here as in test_groupby.py about the function declarations

Comment thread tests/test_resampler.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

DatetimeIndexResampler is not documented, so I don't think we should use it in tests. Also, have functions with Series[int] in declaration, but we can't expect users to do that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Based on the asserts, I think DatetimeIndexResampler is indeed needed for typing purposes 😕 #848

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think those assert should be removed.

But I also think that in pandas, DatetimeIndexResampler, PeriodIndexResampler and TimedeltaIndexResampler should be added to the pandas.api.typing module since pandas.api.typing.DataFrameGroupBy.resample can return those types.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread tests/test_windowing.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same comment about having functions with Series[int] | Series[float] as argument types

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.

2 participants