Don't expose EA.pad_or_backfill to users + switch to DeprecationWarning #54838
Conversation
jorisvandenbossche
added
the
ExtensionArray
| msg = "|".join( | ||
| [ | ||
| "ExtensionArray.fillna added a 'copy' keyword", | ||
| "Series.fillna with 'method' is deprecated", | ||
| ] | ||
| ) | ||
| msg = "Series.fillna with 'method' is deprecated" | ||
| with tm.assert_produces_warning( | ||
| FutureWarning, match=msg, check_stacklevel=False | ||
| FutureWarning, | ||
| match=msg, | ||
| check_stacklevel=False, | ||
| raise_on_extra_warnings=False, |
There was a problem hiding this comment.
assert_produces_warning actually doesn't really work well when there are multiple warnings (it doesn't exactly check the messages, as long as one of the messages matches it seems). Therefore for this test, I tested each warning that should be raised separately (with raise_on_extra_warnings=False), to ensure those are certainly raised.
And for other tests below I went with the easier (but less strict) tm.assert_produces_warning((FutureWarning, DeprecationWarning), ...)
| if limit is not None and limit_area is not None: | ||
| if limit is None: |
There was a problem hiding this comment.
Unless I am missing something, I think this was just wrong before. This code path doesn't use limit, so it should only be used for the cases it was not specified (and this is None).
It worked before as well because the fall back option (the base class implementation) also worked.
And it only started failing now because I removed the limit_area is not None, so the remaining check started to pass when specifying just limit
There was a problem hiding this comment.
cc @mroeschke
There was a problem hiding this comment.
Thanks! This change looks correct to me
| @@ -140,26 +140,59 @@ def test_fillna_frame(self, data_missing): | |||
| def test_fillna_limit_pad(self, data_missing): | |||
| msg = "ExtensionArray.fillna 'method' keyword is deprecated" | |||
| with tm.assert_produces_warning( | |||
| FutureWarning, match=msg, check_stacklevel=False | |||
| DeprecationWarning, | |||
There was a problem hiding this comment.
Not blocking: Would nested tm.assert_produces_warning work here?
There was a problem hiding this comment.
I didn't try that today, but I seem to remember from trying that a while ago that this nesting those asserts don't work at all (although things might have changed)
…s + switch to DeprecationWarning
…o users + switch to DeprecationWarning) (#54851) Backport PR #54838: Don't expose EA.pad_or_backfill to users + switch to DeprecationWarning Co-authored-by: Joris Van den Bossche <[email protected]>
Closes #54831
This does:
ExtensionArray.pad_or_backfilltoExtensionArray._pad_or_backfill(added underscore -> this is only a method that needs to be implemented by developers (EA authors), and not be called by or visible to users)_pad_or_backfillfor when the EA still only hasfillnawith method to a DeprecationWarning, since this is targetting EA authors, we can avoid that initially users see the warninglimit_areakeyword fromExtensionArray._pad_or_backfillsignature, since it is unused (for all EAs except for numy dtypes). Removing it now because otherwise removing it later can give compatibility problems with third party EAs (and then it would need to be deprecated)