CLN: Refactor Index._validate_names() #19246
Conversation
pandas/core/indexes/base.py
Outdated
| if names is not None and name is not None: | ||
| raise TypeError("Can only provide one of `names` and `name`") | ||
| elif names is None and name is None: | ||
|
|
There was a problem hiding this comment.
is there a reason you are splitting this into 2 functions? what is wrong with the original that this is needed?
There was a problem hiding this comment.
Mostly because we need validation logic not only for copying (see #19168). It should reduce repetitions in the code.
There was a problem hiding this comment.
ok, not averse to this, maybe its the spelling and how these are split. if you can clearly define each purpose with a doc-string then all for it.
pandas/core/indexes/base.py
Outdated
| @@ -800,24 +800,29 @@ def __deepcopy__(self, memo=None): | |||
| memo = {} | |||
| return self.copy(deep=True) | |||
|
|
|||
| def _validate_names(self, name=None, names=None, deep=False): | |||
| def _handle_names(self, name=None, names=None, deep=False): | |||
There was a problem hiding this comment.
I see the original issue, but how does this fix things, _validate_names is stilling doing partial validation and partial conversions.
There was a problem hiding this comment.
Well, I agree with that to some extent. But conversion can be removed if Pandas deprecates names argument for MultiIndex as it was proposed in #19168 (comment) .
There was a problem hiding this comment.
well, no, the reason for acceptance is that MI needs to have the same interface as Index.
There was a problem hiding this comment.
But if Index accepts names as an argument, they will be compatible.
pandas/core/indexes/base.py
Outdated
| return name | ||
|
|
||
| return self._validate_names(name=name, names=names) | ||
|
|
There was a problem hiding this comment.
maybe if you these have doc-strings I would be able to tell what is going on here.
jreback
added
Indexing
Codecov Report
@@ Coverage Diff @@
## master #19246 +/- ##
==========================================
- Coverage 91.56% 91.53% -0.04%
==========================================
Files 148 147 -1
Lines 48856 48812 -44
==========================================
- Hits 44733 44678 -55
- Misses 4123 4134 +11
Continue to review full report at Codecov.
|
spacesphere
force-pushed
the
index-names-validation
branch
from
8e9d11c to
3809d87
Compare
|
@jreback , please, review changes |
| names = self._validate_names(name=name, names=names, deep=deep) | ||
|
|
||
| if name is None and names is None: | ||
| from copy import deepcopy |
There was a problem hiding this comment.
import can be at the top. I think write like this
names = self._validate_names(name=name, names=names)
names = deepcopy(self.names) if deep else self.names
to avoid the double check here
| if names is not None and not is_list_like(names): | ||
| raise TypeError("names must be list-like") | ||
|
|
||
| if name is not None: |
There was a problem hiding this comment.
you might be able to remove this as .set_names already handles the list-like / scalar case (or maybe remove it there)
| name = kwargs.get('name') | ||
| names = self._validate_names(name=name, names=names, deep=deep) | ||
|
|
|
@PoppyBagel @jreback : I'm -1 for breaking out the logic like this if we have to duplicate the copying logic. It was perfectly encapsulated in that one function. I'm still in the camp that it just needed to be renamed. |
git diff upstream/master -u -- "*.py" | flake8 --diff