-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
BUG/API: Accessors like .cat raise AttributeError when invalid #9617
BUG/API: Accessors like .cat raise AttributeError when invalid #9617
Conversation
`AttributeError` is really the appropriate error to raise for an invalid attribute. In particular, it is necessary to ensure that tests like `hasattr(s, 'cat')` work consistently on Python 2 and 3: on Python 2, `hasattr(s, 'cat')` will return `False` even if a `TypeError` was raised, but Python 3 more strictly requires `AttributeError`. This is an unfortunate trap that we should avoid. See this discussion in Seaborn for a full report: mwaskom/seaborn#361 (comment) Note that technically, this is an API change, since these accessors (all but `.str`, I think) raised TypeError in the last release. This also suggests another possibility for testing for Series with a Categorical dtype (GH8814): just use `hasattr(s, 'cat')` (at least for Python 2 or pandas >=0.16). CC mwaskom jorisvandenbossche JanSchulz
3b58cee
to
0b02747
Compare
OK, it looks like some existing tests relied on the ability to access columns I suppose can add those to |
This is the definition of when to raise |
I think there can be two ways to look at it:
I think I like the second one better, as
|
If the E.g.
|
This really seems a like a gray area to me as far as standard Python exceptions go:
(Note that the only case I could find for the parenthetical remark about TypeError is when attempting to set an attribute of the built-in To me, the error from What tips me in the direction of AttributeError is that it plays much more nicely with @JanSchulz - we jumped through some hoops to ensure that |
@shoyer ok then. go ahead and merge. |
No strong opinion on this, there are good reasons for both types, so ok with me |
BUG/API: Accessors like .cat raise AttributeError when invalid
thanks for this. btw, if someone wants to do a followup at some point to remove the |
@jreback do we still need to fix those failing tests? I can do a quick followup.... |
I'm working on a followup here -- the build is currently broken! :) |
AttributeError
is really the appropriate error to raise for an invalidattribute. In particular, it is necessary to ensure that tests like
hasattr(s, 'cat')
work consistently on Python 2 and 3: on Python 2,hasattr(s, 'cat')
will returnFalse
even if aTypeError
was raised, butPython 3 more strictly requires
AttributeError
.This is an unfortunate trap that we should avoid. See this discussion in
Seaborn for a full report:
mwaskom/seaborn#361 (comment)
Note that technically, this is an API change, since these accessors (all but
.str
, I think) raised TypeError in the last release.This also suggests another possibility for testing for Series with a
Categorical dtype (#8814): just use
hasattr(s, 'cat')
(at least for Python2 or pandas >=0.16).
CC @mwaskom @jorisvandenbossche @JanSchulz