ENH: Deprecate non-keyword arguments for Index.set_names. by jmholzer · Pull Request #41551 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits11 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- xref Deprecate non-keyword arguments for methods with inplace #41485
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, just some small requests
| @@ -1526,6 +1527,7 @@ def _set_names(self, values, level=None) -> None: |
|---|
| names = property(fset=_set_names, fget=_get_names) |
| @deprecate_nonkeyword_arguments(version="2.0", allowed_args=["self", "names"]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put version=None? I know I'd originally put version="2.0", but then other reviewers expressed a preference for not specifying the exact version.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in my latest commit.
I think these changes also apply to the PR I opened for drop_duplicates (#41500)
Would you like me to make these changes there too?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please, thanks! Your help with these is much appreciated
Comment on lines 357 to 358
| with tm.assert_produces_warning(FutureWarning, match=msg): |
|---|
| idx.set_names(["kind", "year"], None) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also check the result? Again, I'm aware that I didn't do this in the example PR I opened, but then other reviewers suggested it - there's an example in #41511 . Same for the Series test
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, added in the latest commit.
| idx = MultiIndex.from_product([["python", "cobra"], [2018, 2019]]) |
|---|
| msg = ( |
| "In a future version of pandas all arguments of Index.set_names " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as in the other PR, you'll probably need to define this for MultiIndex too (and remove @final) so that the warning shows MultiIndex.set_names
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typing failure in multi.py (because we don't yet have overloads here - but those'll come once these deprecation warnings are in) - for now, could you just silence it? So
diff --git a/pandas/core/indexes/multi.py b/pandas/core/indexes/multi.py index 3c070889c1..269a73cd3c 100644 --- a/pandas/core/indexes/multi.py +++ b/pandas/core/indexes/multi.py @@ -3594,7 +3594,8 @@ class MultiIndex(Index): """ names = self._maybe_match_names(other) if self.names != names:
return self.rename(names)
# Incompatible return value type (got "Optional[MultiIndex]", expected "MultiIndex")
def _maybe_match_names(self, other):return self.rename(names) # type: ignore[return-value] return self
…cate-nonkeyword-args-set_index
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, one suggesting for typing
| @@ -3585,7 +3584,9 @@ def _get_reconciled_name_object(self, other) -> MultiIndex: |
|---|
| """ |
| names = self._maybe_match_names(other) |
| if self.names != names: |
| return self.rename(names) |
| # Incompatible return value type (got "Optional[MultiIndex]", expected |
| # "MultiIndex") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try doing assert isinstance(self, MultiIndex) here
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work, but for things that will be fixed in the future (e.g. this one once all the overloads are in, which will come after positional arguments are deprecated) I think we've generally been ignoring the error
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk, then we can just merge this.
TLouf pushed a commit to TLouf/pandas that referenced this pull request
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request
Labels
Functionality to remove in pandas
Related to the Index class or subclasses