-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Deprecate non-keyword arguments in drop #41486
Deprecate non-keyword arguments in drop #41486
Conversation
doc/source/whatsnew/v1.3.0.rst
Outdated
@@ -647,6 +647,7 @@ Deprecations | |||
- Deprecated setting :attr:`Categorical._codes`, create a new :class:`Categorical` with the desired codes instead (:issue:`40606`) | |||
- Deprecated behavior of :meth:`DatetimeIndex.union` with mixed timezones; in a future version both will be cast to UTC instead of object dtype (:issue:`39328`) | |||
- Deprecated using ``usecols`` with out of bounds indices for ``read_csv`` with ``engine="c"`` (:issue:`25623`) | |||
- Deprecated passing arguments (apart from ``labels``) as positional in :meth:`DataFrame.drop` and :meth:`Series.drop` (:issue:`41485`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per other comments, we could consider deprecating some compatibility parameters on Series at the same time.
r"arguments 'self' and 'labels' will be keyword-only" | ||
) | ||
with tm.assert_produces_warning(FutureWarning, match=msg): | ||
df.drop("a", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an assert that the result is still the same as df.drop("a", axis=1)
(outside of the assert_produces_warning context)
r"arguments 'self' and 'labels' will be keyword-only" | ||
) | ||
with tm.assert_produces_warning(FutureWarning, match=msg): | ||
ser.drop(1, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
FYI @MarcoGorelli a way to avoid the conflicts as we merge is to put the notes slightly randomly in the whatsnew deprecation section (e.g. not one after the other) and/or put blank lines to separate the conflicts, but nbd. |
can u rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MarcoGorelli lgtm pending green.
maybe could also consider (not in this PR) doing the same for Index (although multiindex is an issue as it doesn't have the same signature)
Thanks @simonjayhawkins - Index.drop doesn't have inplace and the return type is always the same so I didn't prioritise it here |
inplace
#41485