-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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): Replace _kwargs in collect method #19618
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #19618 +/- ##
==========================================
- Coverage 79.96% 79.91% -0.05%
==========================================
Files 1536 1536
Lines 211374 211663 +289
Branches 2445 2447 +2
==========================================
+ Hits 169034 169160 +126
- Misses 41785 41948 +163
Partials 555 555 ☔ View full report in Codecov by Sentry. |
This will close #16525 |
post_opt_callback is not intended for public use. It is private. We should instead check the content of |
@ritchie46, commits added |
when I was incorporating @ritchie46's suggestion in disabling CSE in the
collect
method in #19480, I initially usedit didn't complain but produces no performance improvements that I was expecting, until I looked it up and found the keyword to be
comm_subplan_elim
instead.this PR solves this potential pitfull for developers by replacing the _kwargs with spelled out keyword arguments, to let python do the checking for us and raise TypeError if not correct.
typically, unless a method either forwards/delegates the kwargs to another method or it takes whatever, it is reasonable to assume that the method spells out its interface.