Opened 11 years ago

Closed 11 years ago

#19663 closed Cleanup/optimization (fixed)

Sqldiff breaks with PointField declaration

Reported by: Jonathan Liuti Owned by: nobody
Component: Core (Management commands) Version: 1.4
Severity: Normal Keywords: sqldiff, gis
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

I'm on ubuntu 12.04
Django 1.4.3
Postgresql 9.1.7

My model (simplified):

class Estate(models.Model):
    coordinates = models.PointField()
    objects = models.GeoManager()

Syncdb has been run and everything seems to be ok but:

The sqldiff command will fail with the following error when a model contains a PointField:

text = text + '\x1b[%sm' % RESET
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' 

It could be easily fixed by using proper string format syntax instead of '+':

text = '%s\x1b[%sm' % (text,RESET)

Change History (7)

comment:1 by Jonathan Liuti, 11 years ago

Has patch: set

comment:2 by Claude Paroz, 11 years ago

Easy pickings: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Well, sqldiff is not from django core (it's from django-extensions, AFAIK), and we could argue that colorize is not meant to support None for the text parameter, hence the bug could be in sqldiff itself.

However, I think your proposed change is generally good practice. You missed another concatenation on the line below. Tests are also required.

comment:3 by Jonathan Liuti, 11 years ago

yes I'm so used to django_extensions ... and the fact that the trace pointed to a django core file confused me. My bad.

Regarding the potential fix, I already started a talk with Simon Charette on github if you wan't to check on it.

Thanks for the feedback anyway.

Last edited 11 years ago by Jonathan Liuti (previous) (diff)

comment:4 by Jonathan Liuti, 11 years ago

Here is the final pull request with both concatenation fixed and failing test case: https://github.com/django/django/pull/671

comment:5 by anonymous, 11 years ago

Triage Stage: AcceptedReady for checkin

Tests pass and change looks good to me.

comment:6 by Simon Charette, 11 years ago

Fix and test looks good, utils tests pass on python 2.7.3 and 3.2.3 (SQLite).

comment:7 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 04141c525d2b5021b8e9d2452582181097a0850d:

Fixed #19663 -- Allowed None in colorize() text parameter

Thanks Jonathan Liuti for the report and the initial patch, and
Simon Charette for the review.

Note: See TracTickets for help on using tickets.
Back to Top