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 display of Geometry object in admin. #273

Merged
merged 2 commits into from
Nov 16, 2016
Merged

Conversation

dracos
Copy link
Member

@dracos dracos commented Nov 15, 2016

Fixes #272.

@@ -333,7 +333,7 @@ class Meta:
verbose_name_plural = 'geometries'

def __str__(self):
return '%s, polygon %d' % (self.area, self.id)
return '%s, polygon %d' % (smart_text(self.area), self.id)
Copy link
Member Author

Choose a reason for hiding this comment

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

Q: Why didn’t you use u'' as an easier way to fix this?
A: Django 1.8, still supported, still supports Python 3.2, therefore so do we. u'' was only re-added in 3.3.

@stevenday
Copy link
Contributor

Looks good, but looking at a the models.py this is in, there are a few other models that take some text either from a data source or the user and return it from __str__, should we smart_text those too?

@dracos
Copy link
Member Author

dracos commented Nov 16, 2016

Any text coming from a CharField will be a Unicode string, so they’re all already covered (I think I did check them all, double check helpful :) ); the reason this issue arose is because the Geometry __str__ used self.area directly, which caused the area’s __str__ function to be called and return a byte string in Python 2.

@stevenday
Copy link
Contributor

aha, gotcha. I double-checked and I think you're right :shipit:

@dracos dracos merged commit 6b5b1ee into master Nov 16, 2016
@dracos dracos removed the Reviewing label Nov 16, 2016
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