Opened 4 years ago

Closed 4 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


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 Changed 4 years ago by Jonathan Liuti

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 4 years ago by Claude Paroz

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 Changed 4 years ago by Jonathan Liuti

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 4 years ago by Jonathan Liuti (previous) (diff)

comment:4 Changed 4 years ago by Jonathan Liuti

Here is the final pull request with both concatenation fixed and failing test case:

comment:5 Changed 4 years ago by anonymous

Triage Stage: AcceptedReady for checkin

Tests pass and change looks good to me.

comment:6 Changed 4 years ago by Simon Charette

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

comment:7 Changed 4 years ago by Claude Paroz <claude@…>

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