-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Prevent adding new attributes to the accessors .str, .dt and .cat #11575
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
Prevent adding new attributes to the accessors .str, .dt and .cat #11575
Conversation
9a72002
to
344cf2b
Compare
344cf2b
to
b364a15
Compare
I've had some pickling issues with doing this in the past, as |
I agree here with @MaximilianR |
b364a15
to
985a957
Compare
I'm not sure I can follow: as far as I can see, the accessors Currently assigning ( |
This needs a rebase if pulled after #11582... |
@@ -3106,6 +3106,11 @@ def _simple_new(cls, values, name=None, categories=None, ordered=None, **kwargs) | |||
result._reset_identity() | |||
return result | |||
|
|||
# PandasDelegate prevents adding any new attributes as that's what reasonable if used as | |||
# accessor. But we want it again. |
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.
this inherits from PandasObject
so not necessary
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.
At least during my tests it was? It inherits from PandasDelegate:
class CategoricalIndex(Index, PandasDelegate):
985a957
to
8cf43f1
Compare
Ok, took me a while to find a place for the mixin: both core.common and core.base resulted in circular depencies due to the way the StringMethods are defined (
|
fea2a1f
to
2d25b16
Compare
@@ -25,6 +25,7 @@ Enhancements | |||
objects for the ``filepath_or_buffer`` argument. (:issue:`11033`) | |||
- ``DataFrame`` now uses the fields of a ``namedtuple`` as columns, if columns are not supplied (:issue:`11181`) | |||
- Improve the error message displayed in :func:`pandas.io.gbq.to_gbq` when the DataFrame does not match the schema of the destination table (:issue:`11359`) | |||
- Prevent adding new attributes to the accessors .str, .dt and .cat (:issue:`10673`) |
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.
API change section
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 was actually thinking it should go to bugfix: currently you can't get the value back, so the concept is broken and we only make the error more explicit.
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.
ok (put the .str
etal with backticks)
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.
done (to bug and added backticks)
2d25b16
to
bfa6c9b
Compare
BTW: The Series and Indexes are kind of inconsistent:
|
those are not inconsistent, merely implementations. |
I disagree: Consistent would be either:
|
This commit is mostly for the benefit of users who misspell things on the accessors. Assigning to `Series.str`, `Series.dt`, or `Series.cat` was not failing although you couldn't get the value back: ``` In[10]: a = pandas.Series(pandas.Categorical(list("abc"))) In[11]: a.cat.labels = [1,2] In[12]: a.cat.labels AttributeError: 'CategoricalAccessor' object has no attribute 'labels' ``` Now we fail early: ``` In[10]: a = pandas.Series(pandas.Categorical(list("abc"))) In[11]: a.cat.labels = [1,2] AttributeError: You cannot add any new attribute 'labels' ``` Refactor/add a StringAccessorMixin to break a import cycle.
bfa6c9b
to
065415d
Compare
So, move the string part to a new mixin in |
@JanSchulz looks good! ping on green. |
Seems travis is dead?! Ok, I will rebase #11582 on top of this PR, to shortcut that process a bit... |
Prevent adding new attributes to the accessors .str, .dt and .cat
thanks @JanSchulz go ahead and rebase #11582 |
assigning to
Series.str
,Series.dt
, orSeries.cat
was not failingalthough you couldn't get the value back:
Now we fail early:
Closes: #10673