-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
API: Raise when setting name via level #30574
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 1 commit
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 |
---|---|---|
|
@@ -240,6 +240,10 @@ def _outer_indexer(self, left, right): | |
_data: Union[ExtensionArray, np.ndarray] | ||
_id = None | ||
_name: Optional[Hashable] = None | ||
# MultiIndex.levels previously allowed setting the index name. We | ||
# don't allow this anymore, and raise if it happens rather than | ||
# failing silently. | ||
_no_setting_name: bool = False | ||
_comparables = ["name"] | ||
_attributes = ["name"] | ||
_is_numeric_dtype = False | ||
|
@@ -520,6 +524,7 @@ def _simple_new(cls, values, name=None, dtype=None): | |
# we actually set this value too. | ||
result._index_data = values | ||
result._name = name | ||
result._no_setting_name = False | ||
|
||
return result._reset_identity() | ||
|
||
|
@@ -1214,6 +1219,12 @@ def name(self): | |
|
||
@name.setter | ||
def name(self, value): | ||
if self._no_setting_name: | ||
# Used in MultiIndex.levels. | ||
raise RuntimeError( | ||
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. TypeError instead? 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 a RuntimeError is best, since we don't check the type or valuje of |
||
"Cannot set name on a level of a MultiIndex. Use " | ||
"'MultiIndex.set_names' instead." | ||
) | ||
maybe_extract_name(value, None, type(self)) | ||
self._name = value | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,6 +234,7 @@ def _simple_new(cls, array, name, closed=None): | |
result = IntervalMixin.__new__(cls) | ||
result._data = array | ||
result.name = name | ||
result._no_setting_name = False | ||
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. does this need to be set to False in each _simple_new, or can it just use the class-level default? 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. If it’s just on the class, won’t it be shared amongst all instances? I think I should change the type declaration to not have a default. 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 the relevant thing is: when you set 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. Yeah, |
||
result._reset_identity() | ||
return result | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,3 +124,12 @@ def test_get_names_from_levels(): | |
|
||
assert idx.levels[0].name == "a" | ||
assert idx.levels[1].name == "b" | ||
|
||
|
||
def test_setting_names_from_levels_raises(): | ||
idx = pd.MultiIndex.from_product([["a"], [1, 2]], names=["a", "b"]) | ||
with pytest.raises(RuntimeError, match="set_names"): | ||
idx.levels[0].name = "foo" | ||
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 could do something weird like:
is there a less-contrived way in which 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. That raises (correctly, I think). I can add a test for it if you like. |
||
|
||
with pytest.raises(RuntimeError, match="set_names"): | ||
idx.levels[1].name = "foo" |
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.
If
_no_setting_name = False
in the class declaration, isn't it unnecessary here? In both cases the attribute is set to False.