Skip to content

Conversation

charettes
Copy link
Member

Thanks to Jean Gourds for the report.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to use the property decorator syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could do. I usually avoid it in my own code because while the super() getter is easily accessible with super().attribute you to have deal with the property internals (fset) to access the setter.

@timgraham timgraham changed the title Fixed #25867 -- Made sure nested array fields set their wrapped field model. Fixed #25867 -- Fixed a system check crash with nested ArrayFields. Dec 5, 2015
Copy link
Member

Choose a reason for hiding this comment

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

Instead of describing how things failed at one point in time, I think it's more helpful to state the expected behavior, e.g. "Nested ArrayFields are permitted." It would also be helpful to include a comment about what "postgres.E001" is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, this was mostly a copy of test_field_checks above with a nested array field instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@timgraham does this comment makes sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I would say something like "The inner CharField is missing a max_length."

Thanks to Jean Gourds for the report, Tim and Claude for the review.
@charettes charettes merged commit 59b57e6 into django:master Dec 7, 2015
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.

3 participants