BUG: allow kwargs in Styler apply_index and map_index#1725
Conversation
| self, series: Series, /, *args: Any, **kwargs: Any | ||
| ) -> list[Any] | Series: ... | ||
|
|
||
| class _SeriesStrFunc(Protocol): |
There was a problem hiding this comment.
I tried on my end and this protocol thing works but is a bit of overkill for what you are trying to do here. Turns out since args and kwargs here are just Any, you can drop it in exchange for:
Callable[Concatenate[Series, ...]], list[str] | np_ndarray_str | Series[str]]
This will make it a lot cleaner and simpler to understand.
There was a problem hiding this comment.
I tried swapping it out for the Callable that you mentioned and it fails on mypy:
tests/test_styler.py:72: error: Argument 1 to "apply_index" of "Styler" has incompatible type "Callable[[Series[Any]], ndarray[tuple[Any, ...], dtype[str_]]]"; expected "Callable[[[Series[Any], VarArg(Any), KwArg(Any)]], list[str] | ndarray[tuple[Any, ...], dtype[str_]] | Series[str]]" [arg-type]
tests/test_styler.py:77: error: Argument 1 to "apply_index" of "Styler" has incompatible type "Callable[[Series[Any]], Series[str]]"; expected "Callable[[[Series[Any], VarArg(Any), KwArg(Any)]], list[str] | ndarray[tuple[Any, ...], dtype[str_]] | Series[str]]" [arg-type]
tests/test_styler.py:84: error: Argument 1 to "apply_index" of "Styler" has incompatible type "Callable[[Series[Any], str], list[str]]"; expected "Callable[[[Series[Any], VarArg(Any), KwArg(Any)]], list[str] | ndarray[tuple[Any, ...], dtype[str_]] | Series[str]]" [arg-type]
This is a bit of a complex way of doing it, but it aligns with what is used in the other Styler functions (see #921) so I'm inclined to match that style.
There was a problem hiding this comment.
Sorry made a typo above, you should pass:
Callable[Concatenate[Series, ...], list[str] | np_ndarray_str | Series[str]]
(there was a bracket misplaced).
Let me know if that works. The use of Protocol is needed if you have a / in the function definition but since here it works without we should try to go with simplicity and readability if you agree
There was a problem hiding this comment.
@Dr-Irv any guidance on which you see as the most appropriate way forward?
There was a problem hiding this comment.
I ran the tests with the current version of this PR, so I think this should be acceptable. @loicdiridollou do final review.
It's possible that @eosti wasn't using mypy 2.0.0 or higher. The tests in CI were run with mypy 2.1.
There was a problem hiding this comment.
Yes this works, I was just looking for a simpler solution (suggested above), compared to the Protocol class.
Eventually let's not block this one, this works well and should fix users errors.
loicdiridollou
left a comment
loicdiridollou
left a comment
There was a problem hiding this comment.
There is an opportunity to simplify here, the tests though are looking good!
| check(assert_type(DF.style.apply_index(f1), Styler), Styler) | ||
|
|
||
| # GH 1723 | ||
| def highlight_odd(index: pd.Series, /, color: str) -> list[str]: |
There was a problem hiding this comment.
No need for the /, it should work without it since it is hardly a usecase you see in production code.
There was a problem hiding this comment.
The /s were added in response to #919, but I tried it out without the /s and it worked fine... So I guess something changed in the last two years; I'll remove these.
|
Hey @eosti, |
|
Thanks for the ping, this completely slipped my mind! I'll take a look now. |
loicdiridollou
left a comment
loicdiridollou
left a comment
There was a problem hiding this comment.
Nice one @eosti, thanks for your contribution!
3 participants
Adds ability to pass a function with
**kwargsto Stylerapply_indexandmap_index. Theapplyandmapfunctions already supported this.apply_*andmapfunctions do not accept**kwargs#1723assert_type()to assert the type of any return value)AGENTS.md.