Sqldiff breaks with PointField declaration

[Jonathan Liuti]

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)

[Jonathan Liuti]

Pull request is here : ​https://github.com/django/django/pull/670

[Claude Paroz]

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.

[Jonathan Liuti]

yes I'm so used to django_extensions ... and the fact that the trace pointed to django core files 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.

[Jonathan Liuti]

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

[anonymous]

Tests pass and change looks good to me.

[Simon Charette]

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

[Claude Paroz <claude@…>]

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.