Skip to content

Conversation

msullivan
Copy link
Collaborator

The dataclasses plugin incorrectly expected json to preserve
the ordering of its dictionaries, which led to spurious errors
about "Attributes without a default cannot follow attributes with one".

Worse, the errors are reported in the wrong file: they use the line
number of the attribute but the file of the dataclass being checked!

Fix the spurious error by serializing attributes in a list, since we
care about the order.

Reporting this error for attributes in parent classes is actually
useful, since multiple inheritance can cause this error. Report the
error at the class definition site causing the problem instead.
(The error message here could certainly be improved, but right
now I just want to fix the "wrong file" bugs, which are I think
the worst kind of bugs after crashes.)

The dataclasses plugin incorrectly expected json to preserve
the ordering of its dictionaries, which led to spurious errors
about "Attributes without a default cannot follow attributes with one".

Worse, the errors are reported in the wrong file: they use the line
number of the attribute but the file of the dataclass being checked!

Fix the spurious error by serializing attributes in a list, since we
care about the order.

Reporting this error for attributes in parent classes is actually
useful, since multiple inheritance can cause this error. Report the
error at the class definition site causing the problem instead.
(The error message here could certainly be improved, but right
now I just want to fix the "wrong file" bugs, which are I think
the worst kind of bugs after crashes.)
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.

LGTM, thanks for the quick fix!

@msullivan msullivan merged commit 8b86349 into master Oct 2, 2019
@msullivan msullivan deleted the dataclass-mess branch October 2, 2019 17:42
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