Code

#19663 closed Cleanup/optimization (fixed)

Sqldiff breaks with PointField declaration

Reported by: jli 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)

Attachments (0)

Change History (7)

comment:1 Changed 18 months ago by jli

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

comment:2 Changed 18 months ago by claudep

  • Easy pickings set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/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 18 months ago by jli

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 18 months ago by jli (previous) (diff)

comment:4 Changed 18 months ago by jli

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

comment:5 Changed 18 months ago by anonymous

  • Triage Stage changed from Accepted to Ready for checkin

Tests pass and change looks good to me.

comment:6 Changed 18 months ago by charettes

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

comment:7 Changed 18 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.