KEMBAR78
Fix dataclass inherited field ordering by jstasiak · Pull Request #5877 · python/mypy · GitHub
Skip to content

Fix dataclass inherited field ordering #5877

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 9, 2018
Merged

Fix dataclass inherited field ordering #5877

merged 1 commit into from
Nov 9, 2018

Conversation

@jstasiak
Copy link
Contributor

@jstasiak jstasiak commented Nov 8, 2018 •
edited
Loading

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 fix-dataclass-field-ordering branch from 27f1fee to ac93608 Compare November 8, 2018 19:08
ilevkivskyi
ilevkivskyi reviewed Nov 8, 2018
View reviewed changes
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mypy/plugins/dataclasses.py
# 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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fix-dataclass-field-ordering branch 2 times, most recently from ac4bb0d to ac97971 Compare November 8, 2018 19:54
@jstasiak
Copy link
Contributor Author

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.

ilevkivskyi
ilevkivskyi reviewed Nov 9, 2018
View reviewed changes
@jstasiak
Fix dataclass inherited field ordering
b961e4a 
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 python#5681.
@jstasiak jstasiak force-pushed the fix-dataclass-field-ordering branch from ac97971 to b961e4a Compare November 9, 2018 14:50
@ilevkivskyi ilevkivskyi merged commit 52d937a into python:master Nov 9, 2018
@jstasiak jstasiak deleted the fix-dataclass-field-ordering branch November 9, 2018 15:09
@efimk-lu
Copy link

Hey @jstasiak , @ilevkivskyi ,

For some reason I keep getting the same issue, I'm using my 0.670.

class AbstractModel(ABC):
   pass
@dataclass(unsafe_hash=True)
class Model(AbstractModel):
    transaction_id: str
    message_id: Optional[str]
    raw: dict = field(hash=False) 
    customer: str
    resource_name: Optional[str] = field(default=None)

@dataclass(unsafe_hash=True)
class TestModel(SpanModel):
    name: str  # <-- Getting here  error: Attributes without a default cannot follow attributes with one

@jstasiak
Copy link
Contributor Author

Can you provide a complete piece of code that works at runtime yet triggers this? I think there are some pieces missing here (no SpanModel definition).

@efimk-lu
Copy link

@jstasiak it looks like it was an internal issue. Solved, Tx.

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

Reviewers

@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jstasiak @efimk-lu @ilevkivskyi