-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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): Add missing read_database
overload
#16229
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16229 +/- ##
=======================================
Coverage 80.85% 80.86%
=======================================
Files 1394 1394
Lines 179955 179955
Branches 2909 2909
=======================================
+ Hits 145508 145525 +17
+ Misses 33942 33925 -17
Partials 505 505 ☔ View full report in Codecov by Sentry. |
Hmm, at first glance it seems odd to be overloading our typing so that something which customises it (unknown to us) doesn't have to set its own, though perhaps I'm missing something. Feels like it could be a slippery slope where we end up unnecessarily (from our perspective) overloading some enormous number of functions just in case somebody wants to overload them and not do the typing on their end 😅 Could you better-clarify why it couldn't be done on your side instead? |
3813d25
to
e71f413
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is indeed the correct way to do overloads on booleans - I have done this fix in some other places already.
@alexander-beedie you can check out this blog for some of the quirks of Python overloads, they explain it relatively well:
https://medium.com/analytics-vidhya/making-sense-of-typing-overload-437e6deecade
read_database
overloadread_database
overload
Co-authored-by: Stijn de Gooijer <[email protected]>
Co-authored-by: Stijn de Gooijer <[email protected]>
This PR adds an overload when
iter_batches
is abool
, as opposed toLiteral[True]
orLiteral[False]
, most of the time in Polars, overloads like this aren't super helpful, as you're actually just passing inTrue
/False
. However, with top-level functions likeread_database
, you might want to have a wrapper over it. This is basically the situation that I am in, where I have a custom version ofread_database
, with some additional functionality around retrieving the DB connection, and so want to have my own overloads on that function forDataFrame
, andIterable[DataFrame]
, to make this possible there does need to be an overload for the non-literal case, at least with Pyright.Here's basically an example of the issue I'm running into.