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

fix(python): Change method op type hints to match dunder op type hints #18448

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ts826848
Copy link

Type hints for operators were refined in #13635 0, but the refinements were generally not propagated to the method equivalents (pow() being the sole exception). This can result in equivalent expressions being treated differently by type checkers, especially if stricter settings are used that discourage/disallow the use of Any.

This commit changes the type hints for the method equivalents to match those for the operators. This should ensure resulting types are consistent regardless of whether one uses infix operators or their method equivalents.

This also has the side benefit of making the documentation a bit more consistent as the type hint more closely matches the stated type of the argument in the docstring.

Type hints for operators were refined in pola-rs#13635 [0], but the refinements
were generally not propagated to the method equivalents (pow() being the
sole exception). This can result in equivalent expressions being treated
differently by type checkers, especially if stricter settings are used
that discourage/disallow the use of Any.

This commit changes the type hints for the method equivalents to match
those for the operators. This should ensure resulting types are
consistent regardless of whether one uses infix operators or their
method equivalents.

This also has the side benefit of making the documentation a bit more
consistent as the type hint more closely matches the stated type of the
argument in the docstring.

[0]: pola-rs@1c417e4
@github-actions github-actions bot added fix Bug fix python Related to Python Polars labels Aug 28, 2024
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.85%. Comparing base (b2550a0) to head (f842b15).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18448      +/-   ##
==========================================
- Coverage   79.86%   79.85%   -0.01%     
==========================================
  Files        1496     1496              
  Lines      200364   200364              
  Branches     2867     2867              
==========================================
- Hits       160012   160005       -7     
- Misses      39806    39813       +7     
  Partials      546      546              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ts826848
Copy link
Author

Whoops, looks like I forgot to run the pre-commit locally so I didn't catch the type errors before creating the PR. My mistake :/

Not sure what the best approach is for dealing with the mypy error. #type: ignoreing it is an option since the reduction is (approximately) equivalent to self & others[0] & others[1] & ... and that is well-typed, but I'm not certain there's a better approach. Thoughts?

I also notice the second argument to reduce is (self, *others) for and_ and (self,) + others for or_. Looks like #13635 changed on but not the other. Should I change or_ to match and_ in this PR or in a different one, if at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant