REF: use unpack_zerodim_and_defer on EA methods #34042
Conversation
| @@ -1181,10 +1181,6 @@ def convert_values(param): | |||
| ovalues = [param] * len(self) | |||
| return ovalues | |||
|
|
|||
| if isinstance(other, (ABCSeries, ABCIndexClass)): | |||
| # rely on pandas to unbox and dispatch to us | |||
| return NotImplemented | |||
There was a problem hiding this comment.
Personally, I wouldn't use the decorator here. This class is meant to be used by external EA authors (and in that way also serves somewhat as documentation), and it is not used internally (except for tests).
I find the "easy understandability" in this case more important than the small code duplication (so it is clear to EA authors this method is returning NotImplemented for pandas objects.
And you can add ABCDataFrame to the check to make it correct (and we should have a test to catch this ..)
There was a problem hiding this comment.
I think its fine to make this change. why have extra-boilerplate code
There was a problem hiding this comment.
why have extra-boilerplate code
For the reason I gave in the above comment
There was a problem hiding this comment.
Can we simply remove it here? (or actually give arguments for why you want to change it)
This is basically a toy/helper implementation that is not used internally.
(I am completely fine with the rest of the PR)
There was a problem hiding this comment.
@jbrockmendel I am ok with this here, but I suppose @jorisvandenbossche is stronger here, I guess revert this part.; though this makes things a bit inconsistent across pandas which is odd
jreback
added
Clean
ExtensionArray
|
@jbrockmendel I am +1 on this, can you merge master. If @jorisvandenbossche object to the array/base.py impl then ok but otherwise lgtm. |
| @@ -1181,10 +1181,6 @@ def convert_values(param): | |||
| ovalues = [param] * len(self) | |||
| return ovalues | |||
|
|
|||
| if isinstance(other, (ABCSeries, ABCIndexClass)): | |||
| # rely on pandas to unbox and dispatch to us | |||
| return NotImplemented | |||
There was a problem hiding this comment.
Can we simply remove it here? (or actually give arguments for why you want to change it)
This is basically a toy/helper implementation that is not used internally.
(I am completely fine with the rest of the PR)
|
can u rebase and add the suggested test |
|
Mothballing |
jbrockmendel
added
the
Mothballed
|
I reopened this because I think it are useful changes. I only changed it back in the base implementation (which is not used in pandas), as I suggested above. |
jorisvandenbossche
removed
the
Mothballed
jbrockmendel
force-pushed
the
boilerplate-4
branch
from
7796934 to
5e8269d
Compare
|
@jorisvandenbossche i just rebased and force-pushed, might have undone the commit you pushed. feel free to re-do that |
jorisvandenbossche
force-pushed
the
boilerplate-4
branch
from
5e8269d to
7796934
Compare
|
I forced pushed again my branch, but it still needs a merge of master. |
|
And also merged master |
|
lgtm. @jorisvandenbossche merge when ready. (or ping and i will) |
No description provided.