Skip to content

allow args and kwargs in groupby.apply #1268

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Dr-Irv
Copy link
Collaborator

@Dr-Irv Dr-Irv commented Jun 28, 2025

@Dr-Irv Dr-Irv requested a review from twoertwein June 28, 2025 20:46
@schorlton-bugseq
Copy link

Thank you!

Copy link
Member

@twoertwein twoertwein left a comment

Choose a reason for hiding this comment

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

Two small optional changes (happy to merge without them):

  • include_groups is not deprecated, only include_groups=True, I would be fine keeping the test and adding include_groups: Literal[False] in the signature
  • it might be nice to have a test where a kwargs/args is incompatible with the func

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Jun 30, 2025

Two small optional changes (happy to merge without them):

  • include_groups is not deprecated, only include_groups=True, I would be fine keeping the test and adding include_groups: Literal[False] in the signature

Yes, but there are 2 issues with that.

  1. The argument will become irrelevant since only a value of False is accepted. So we shouldn't encourage its use.
  2. If we use ParamSpec, we can't have a keyword argument between *args and **kwargs in the typing stubs. See Is `typing.Protocol` the only way to concatenate the keyword-only arguments in `ParamSpec`? python/typing#1905
  • it might be nice to have a test where a kwargs/args is incompatible with the func

Yes, although I'm not sure how to write one! I just wanted to handle the use case reported.

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.

GroupBy.apply: error: No overloads for "apply" match the provided arguments (reportCallIssue)
3 participants