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

Fix dataclass inherited field ordering #5877

Merged
merged 1 commit into from Nov 9, 2018

Conversation

Projects
None yet
2 participants
@jstasiak
Copy link
Contributor

jstasiak commented Nov 8, 2018

Fixes #5681.

This is how the dataclasses module behaves on Python 3.7.0:

% cat test.py
from dataclasses import dataclass

@dataclass
class Mammal:
    age: int

@dataclass
class Person(Mammal):
    name: str
    age: int

@dataclass
class SpecialPerson(Person):
    special_factor: float

@dataclass
class ExtraSpecialPerson(SpecialPerson):
    age: int
    special_factor: float
    name: str

print(Person(32, 'John'))
print(SpecialPerson(21, 'John', 0.5))
print(ExtraSpecialPerson(21, 'John', 0.5))

% python test.py
Person(age=32, name='John')
SpecialPerson(age=21, name='John', special_factor=0.5)
ExtraSpecialPerson(age=21, name='John', special_factor=0.5)

@jstasiak jstasiak force-pushed the jstasiak:fix-dataclass-field-ordering branch from 27f1fee to ac93608 Nov 8, 2018

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for PR! This looks good, I have few comments below. Also I have two questions:

  • Is this specific to dataclasses, or attrs plugin should be also updated to give correct order?
  • Do we have an issue about this? If yes, I would add Fixes #XXXX to the top of PR description.
Show resolved Hide resolved test-data/unit/check-incremental.test Outdated
# reverse MRO, not simply MRO.
# See https://docs.python.org/3/library/dataclasses.html#inheritance for
# details.
(attr,) = [a for a in all_attrs if a.name == name]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 8, 2018

Collaborator

Will this not crash if there are two attrs with the same name? Do we have a test for this?

This comment has been minimized.

@jstasiak

jstasiak Nov 8, 2018

Contributor

I don't believe it can happen within a single class and with multiple classes the if name not in known_attrs block either adds an attribute that wasn't there or, in case of an attribute being overridden, it reorders attributes without adding duplicates.

The way this assignment is written is to express the fact that we strictly expect single attribute with the name name here, I can't think of a situation in which this condition is not met, any possible name clashes are covered by the tests already.

@jstasiak jstasiak force-pushed the jstasiak:fix-dataclass-field-ordering branch 2 times, most recently from ac4bb0d to ac97971 Nov 8, 2018

@jstasiak

This comment has been minimized.

Copy link
Contributor

jstasiak commented Nov 8, 2018

I have no idea about attrs and the attrs plugin, I've never used it. Turns out there was an issue filled about more or less the same thing - I updated the description and the commit message.

Fix dataclass inherited field ordering
This is how the dataclasses module behaves on Python 3.7.0:

    % cat test.py
    from dataclasses import dataclass

    @DataClass
    class Mammal:
        age: int

    @DataClass
    class Person(Mammal):
        name: str
        age: int

    @DataClass
    class SpecialPerson(Person):
        special_factor: float

    @DataClass
    class ExtraSpecialPerson(SpecialPerson):
        age: int
        special_factor: float
        name: str

    print(Person(32, 'John'))
    print(SpecialPerson(21, 'John', 0.5))
    print(ExtraSpecialPerson(21, 'John', 0.5))

    % python test.py
    Person(age=32, name='John')
    SpecialPerson(age=21, name='John', special_factor=0.5)
    ExtraSpecialPerson(age=21, name='John', special_factor=0.5)

Fixes #5681.

@jstasiak jstasiak force-pushed the jstasiak:fix-dataclass-field-ordering branch from ac97971 to b961e4a Nov 9, 2018

@ilevkivskyi ilevkivskyi merged commit 52d937a into python:master Nov 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jstasiak jstasiak deleted the jstasiak:fix-dataclass-field-ordering branch Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment