-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Deprecate non-keyword arguments for drop_duplicates. #41500
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
Changes from 13 commits
ebd40aa
8cb7645
fa6574c
d7c341a
19aa589
448e8a1
0d54ca7
463c37a
c0d3d34
2cb482f
09fe413
03d0330
be8393d
fbf70a2
937d9e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -471,3 +471,21 @@ def test_drop_duplicates_non_boolean_ignore_index(arg): | |
msg = '^For argument "ignore_index" expected type bool, received type .*.$' | ||
with pytest.raises(ValueError, match=msg): | ||
df.drop_duplicates(ignore_index=arg) | ||
|
||
|
||
def test_drop_duplicates_pos_args_deprecation(): | ||
# GH#41485 | ||
df = DataFrame({"a": [1, 1, 2], "b": [1, 1, 3], "c": [1, 1, 3]}) | ||
|
||
msg = ( | ||
"In a future version of pandas all arguments of " | ||
"DataFrame.drop_duplicates except for the argument 'subset' " | ||
"will be keyword-only" | ||
) | ||
|
||
with tm.assert_produces_warning(FutureWarning, match=msg): | ||
result = df.drop_duplicates(["b", "c"], "last") | ||
|
||
expected = DataFrame({"a": [1, 2], "b": [1, 3], "c": [1, 3]}, index=[1, 2]) | ||
|
||
tm.assert_frame_equal(expected, result) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can remove all the extra newlines here, this test could read as a single paragraph There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this change to all my tests in fbf70a2, they now all read as one paragraph. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1738,3 +1738,22 @@ def test_construct_from_memoryview(klass, extra_kwargs): | |
result = klass(memoryview(np.arange(2000, 2005)), **extra_kwargs) | ||
expected = klass(range(2000, 2005), **extra_kwargs) | ||
tm.assert_index_equal(result, expected) | ||
|
||
|
||
def test_drop_duplicates_pos_args_deprecation(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you also put the issue number here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added this change in 2cb482f. |
||
# GH#41485 | ||
|
||
idx = Index([1, 2, 3, 1]) | ||
|
||
msg = ( | ||
"In a future version of pandas all arguments of " | ||
"Index.drop_duplicates will be keyword-only" | ||
) | ||
|
||
with tm.assert_produces_warning(FutureWarning, match=msg): | ||
idx.drop_duplicates("last") | ||
result = idx.drop_duplicates("last") | ||
|
||
expected = Index([2, 3, 1]) | ||
|
||
tm.assert_index_equal(expected, result) |
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.
is it necessary to annotate self as
_IndexT
? If think you should be able to type the return type asMultiIndex
and leaveself
untyped, seeMultiIndex.dropna
Also, does
super().drop_duplicates(keep=keep)
not work?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.
Finally,
str
should be fine in this fine in this file, I think it's only used inpandas/core/indexes/base.py
because there there'sstr = CachedAccessor("str", StringMethods)
(note to self: there should probably be a pre-commit rule for this)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.
I made all the changes suggested in both comments, they are in fbf70a2.