-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
Implement Delegate parts of #24024 #24463
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24463 +/- ##
==========================================
- Coverage 92.3% 92.29% -0.01%
==========================================
Files 163 163
Lines 51969 51937 -32
==========================================
- Hits 47968 47936 -32
Misses 4001 4001
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #24463 +/- ##
==========================================
- Coverage 92.3% 92.29% -0.01%
==========================================
Files 163 163
Lines 51969 51937 -32
==========================================
- Hits 47968 47936 -32
Misses 4001 4001
Continue to review full report at Codecov.
|
The delegation changes seem relatively straightforward. @jorisvandenbossche @jreback do you have any problems with doing them in #24024? |
I think this uses |
My point was that if people have had a chance to review the dispatching in #24024, and are happy with it, then it needn't be split off here (especially when the changes here aren't what will end up being in master when eadata is removed). |
well that's the problem. that PR is so big that its not easy to review it at all. |
I was under the impression that people had already looked at the dispatching parts. Was that wrong? |
@TomAugspurger there is so much going on there it will take enormous effort and time to do it this way. splitting is much better. |
I don't follow your logic. I'm trying to reduce wasted effort, by not
splitting things that have already been reviewed (though maybe they haven't
been reviewed) and by not having the intermediate _eadata in too many
places.
…On Fri, Dec 28, 2018 at 7:54 AM Jeff Reback ***@***.***> wrote:
@TomAugspurger <https://github.com/TomAugspurger> there is so much going
on there it will take enormous effort and time to do it this way. splitting
is much better.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24463 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIslxcLCC1-4veGGUp026QwBt3Z0Tks5u9iKLgaJpZM4ZjYzs>
.
|
The problem is if its in the original, who and how was it reviewed? its not clear at all. sure maybe it was looked at briefly, but leaving it there is the worst scenario. you have to constantly scan for changes. I haven't even looked at your PR fully once. Its simply too many changes. |
I guess I was wrong then. Feel free to proceed how you see best.
…On Fri, Dec 28, 2018 at 8:01 AM Jeff Reback ***@***.***> wrote:
I don't follow your logic. I'm trying to reduce wasted effort, by not
splitting things that have already been reviewed (though maybe they haven't
been reviewed) and by not having the intermediate _eadata in too many
places.
The problem is if its in the original, *who* and *how* was it reviewed?
its not clear at all. sure maybe it was looked at briefly, but leaving it
there is the worst scenario. you have to constantly scan for changes. I
haven't even looked at your PR fully once. Its simply too many changes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24463 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHImaP2gmUCF6MY0kgBps8y6ikqH5lks5u9iQkgaJpZM4ZjYzs>
.
|
thanks @jbrockmendel and @TomAugspurger |
This one got a little bit messy because until we move all the way away from inheritance there are some methods that need to be overwritten, so the
@delegate_names
calls are a bit more verbose than in #24024.