Skip to content
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

Python 3 __dict__ inheritance #1777

Merged
merged 1 commit into from
Sep 18, 2015
Merged

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Sep 11, 2015

For whatever reason, the __dict__ property does not get inherited, so it needs to be re-implemented in a few classes.

@QuLogic QuLogic mentioned this pull request Sep 11, 2015
14 tasks
@rhattersley
Copy link
Member

It might be better to fix this by defining __slots__ = () for these classes (instead of defining the __dict__ property.)

@QuLogic
Copy link
Member Author

QuLogic commented Sep 12, 2015

I'm not sure that would do anything as I think that's already true:

In [1]: from collections import namedtuple
In [2]: Foo = namedtuple('Foo', 'foo bar')
In [3]: class Bar(Foo): __eq__ = lambda self, other: True

In [4]: f = Foo(1, 2)
In [5]: f.__slots__
Out[5]: ()

In [6]: b = Bar(1, 2)
In [7]: b.__slots__
Out[7]: ()

@QuLogic QuLogic modified the milestone: v1.9 Sep 12, 2015
@rhattersley
Copy link
Member

I'm not sure that would do anything as I think that's already true:

Without getting in to the subtleties of it all (I'd have to spend a while researching them!):

>>> class Wibble(Foo): pass
>>> Wibble(1, 2).__dict__
{}

>>> class Wobble(Foo): __slots__ = ()
>>> Wobble(1, 2).__dict__
OrderedDict([('foo', 1), ('bar', 2)])

@QuLogic
Copy link
Member Author

QuLogic commented Sep 14, 2015

Oh, of course, I tested it by setting __slots__ after class creation and not during.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 15, 2015

OK, switched to __slots__, but only where necessary. Does it make sense to add it on all the namedtuple derivatives?

@rhattersley
Copy link
Member

Does it make sense to add it on all the namedtuple derivatives?

Not sure... probably yes. It appeals from a consistency point of view, and the derivatives were all intended to be immutable.

Out of interest, how did you spot the need for this PR? (I'm trying to recreate the relevant test failures so I can better understand the possible impact.)

@rhattersley
Copy link
Member

Out of interest, how did you spot the need for this PR? (I'm trying to recreate the relevant test failures so I can better understand the possible impact.)

I think I've tracked this down to things like _CoordMetaData.__eq__ failing because its calls to self._asdict() and other._asdict() return {} on Python 3 without this PR.

@rhattersley
Copy link
Member

Does it make sense to add it on all the namedtuple derivatives?

Not sure... probably yes

Enough vagueness! If I have to choose an option, then 👍 for slots. 😉

@QuLogic
Copy link
Member Author

QuLogic commented Sep 16, 2015

I added it to everything except ArrayStructure, where it cannot be added because the __new__ sets the self.size attribute.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 16, 2015

Tests currently broken by ContinuumIO/anaconda-issues#445.

@rhattersley
Copy link
Member

I added it to everything except ArrayStructure, where it cannot be added because the new sets the self.size attribute.

We could define size as a property instead?

This should restore the __dict__ property that is necessary in some
places.
@QuLogic
Copy link
Member Author

QuLogic commented Sep 17, 2015

Yep, seems to work alright.

@rhattersley
Copy link
Member

Looks good. 👍

rhattersley added a commit that referenced this pull request Sep 18, 2015
@rhattersley rhattersley merged commit 6c0267f into SciTools:master Sep 18, 2015
@QuLogic QuLogic deleted the py3k-inheritance branch September 18, 2015 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants