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

Fix relational fields without to_fields attribute. #3635

Merged
merged 3 commits into from Nov 16, 2015

Conversation

martinhill
Copy link

@martinhill martinhill commented Nov 14, 2015

Fixes issue 3634

@@ -92,7 +92,7 @@ def _get_fields(opts):


def _get_to_field(field):
return field.to_fields[0] if field.to_fields else None
return field.to_fields[0] if hasattr(field, 'to_fields') else None
Copy link
Collaborator

@xordoquy xordoquy Nov 14, 2015

Choose a reason for hiding this comment

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

Actually, the new line doesn't check whether to_fields is None or not

@martinhill
Copy link
Author

martinhill commented Nov 15, 2015

Sorry for multiple commits, that should be the final one.

@xordoquy xordoquy added this to the 3.3.2 Release milestone Nov 15, 2015
@xordoquy xordoquy changed the title fix for issue #3634 Relation fields no to_fields attribute attribute break serializer Nov 15, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Nov 16, 2015

Wondering whether we need a test here - most likely we do

@@ -92,7 +92,7 @@ def _get_fields(opts):


def _get_to_field(field):
return field.to_fields[0] if field.to_fields else None
return getattr(field, 'to_fields', None) and field.to_fields[0]
Copy link
Member

@tomchristie tomchristie Nov 16, 2015

Choose a reason for hiding this comment

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

Should we us hasattr instead?

Copy link
Collaborator

@xordoquy xordoquy Nov 16, 2015

Choose a reason for hiding this comment

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

@tomchristie no, cf my previous outdated comment

Copy link
Member

@tomchristie tomchristie Nov 16, 2015

Choose a reason for hiding this comment

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

Ack - ignore my comment there. Incorrect.

@tomchristie
Copy link
Member

tomchristie commented Nov 16, 2015

Wondering whether we need a test here - most likely we do

For simple cases I'm kinda okay with us simply switching out a more brittle style for a safer style. A test case would be appreciated, but would probably accept either way.

@martinhill
Copy link
Author

martinhill commented Nov 16, 2015

Agreed – I could definitely add a test case, but might take a couple days.

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

Successfully merging this pull request may close these issues.

None yet

3 participants