WIP: ensure names are pinned correctly #24429
Conversation
|
Hello @jbrockmendel! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on January 10, 2019 at 02:41 Hours UTC |
|
Are we sure we want pandas objects to have a metaclass? Any Python (sub)class may only have one metaclass, so by implementing one in pandas itself we would restrict users of pandas from implementing their own metaclass on subclassed pandas objects, when/if they need it. I think that’s an important issue and makes me leaning on -1 on this. Can’t these checks not be implemented using other methods, e.g. using simple pytests tests? |
Yah, I came to the same conclusion and changed the name-checking bit to go into tests.base. The class inequality thing I think is neat, but is really unrelated to the rest of the PR. |
IMO, it’s unconventional, so usage of this pattern will surprise users and require them to dig (quite deep) into the pandas source code to understand what the comparisons mean. I think just using issubclass directly is much better/clearer, and will also leave the metaclass slot open for users to use in their own subclasses. So, I’m -1 on adding the metaclass to PandasObject... |
While I might want to revisit it at some point, it is definitely not relevant to this PR, so just reverted it. |
pandas/tests/test_base.py
Outdated
| continue | ||
| if name in ignore: | ||
| continue | ||
| if inspect.ismethod(attr) or isinstance(attr, property): |
There was a problem hiding this comment.
In Python3 methods on (non-instantiated) classes are functions, so for those ismethod will fail. You need to add inspect.isfunction.
| if 'Subclassed' in cls.__name__: | ||
| # dummy classes defined in tests | ||
| return | ||
| for name in dir(cls): |
There was a problem hiding this comment.
This iteration looks heavy. What time does it take to run through all cases defined in the test_pinned_names parametrization? Should test_pinned_names below get a pytest slow marker?
Would it not be better to do specific tests for the cases where methods are generated dynamically instead of this broader test?
There was a problem hiding this comment.
What time does it take to run through all cases defined in the test_pinned_names parametrization? Should test_pinned_names below get a pytest slow marker?
Locally python -m pytest pandas/tests/test_base.py -k test_pinned_names takes 1.02 seconds.
Would it not be better to do specific tests for the cases where methods are generated dynamically instead of this broader test?
No, that's an invitation to have things fall through the cracks down the road.
jbrockmendel
changed the title
Codecov Report
@@ Coverage Diff @@
## master #24429 +/- ##
===========================================
- Coverage 92.31% 43% -49.31%
===========================================
Files 163 163
Lines 51987 51987
===========================================
- Hits 47990 22356 -25634
- Misses 3997 29631 +25634
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24429 +/- ##
==========================================
+ Coverage 92.38% 92.38% +<.01%
==========================================
Files 166 166
Lines 52310 52324 +14
==========================================
+ Hits 48326 48341 +15
+ Misses 3984 3983 -1
Continue to review full report at Codecov.
|
|
looks ok, but this raises a number of questions about existing behaviors as you have noted. |
gfyoung
added
Bug
Internals
|
Ref #23551 need to see what it would take to handle that here |
|
Do we want to move forward with something like this? If not let’s close |
Discussed in #24397, this defines a metaclass for PandasObject that does two things:
__name__matches the namespace name.NotImplementedchecks toif type(self) < type(other)The
__name__have a whole bunch that currently fail, will whittle those down.