DEPR: allowing non-class returns for _constructor etc #52420
Conversation
| warnings.warn( | ||
| "pandas subclass support is deprecating allowing _constructor " | ||
| "that does not return a class.", | ||
| FutureWarning, | ||
| stacklevel=find_stack_level(), | ||
| ) |
There was a problem hiding this comment.
does this raise a warning all the time, with no option to silence it?
just wondering if there's a way to do this without having to always raise a warning, and if not, if the warning should advise to just use filterwarnings to suppress the message because there's no workaround
There was a problem hiding this comment.
does this raise a warning all the time, with no option to silence it?
Yes. I tried having it check and only raise if _constructor_foo doesn't return a class but couldn't get that working.
just wondering if there's a way to do this without having to always raise a warning, and if not, if the warning should advise to just use filterwarnings to suppress the message because there's no workaround
Makes sense. Or as mentioned in the OP this could be doc-only.
mroeschke
added
Deprecate
There was a problem hiding this comment.
My reading of the issue #51772 is not that there is any kind of agreement that we actually want to do this.
I actually asked clarification of the motivation and provided arguments why I think this is not such a useful change (#51772 (comment)), and since then got not even a reply to that.
We've had this discussion and I decline to go around in circles with you on this. |
|
+1 on deprecating and making these class only when we originally did this there was NO intention or reason to ever make these callable. it's not necessary and just too flexible. |
Sorry to chime in, but can I ask that we be a little more empathetic in written conversation please? This kind of tone can be off-putting to any newcomer who might be reading along (as well as to anyone else, really) I'm well aware that I'm not always the best or most empathetic communicator myself, I'm just saying - let's all try to work on this |
ah, why am I so bad at typing, of course I meant to write "I'm not always" 🤦 have edited Anyway, back to the issue at hand. I've re-read through #51772. If this was never meant to work in the first place, and it was never documented to work, then it seems perfectly fine to deprecate. Especially if it simplifies the codebase and if it's not technically difficult to workaround in geopandas Regarding the point that it'd be easy to write a code check to ensure that |
|
How do we move forwards here? @jorisvandenbossche would you want a vote on this? A discussion in the community call? |
|
shall we hold off from merging main until we find a way forwards in the discussion in the issue? |
Is the CI clogged? If so, sure. Otherwise I don't see much downside to keeping this up-to-date. Easier to rebase periodically than all at once. |
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
+1 on moving forward with this especially if there's a reasonable workaround in geopandas |
|
@mroeschke the substantial discussion is at #51772 |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.I'd be OK with leaving out the
__init_subclass__bit and just have this be in the docs.