Code

Opened 6 months ago

Closed 3 months ago

Last modified 3 months ago

#21323 closed Cleanup/optimization (fixed)

Allow migrations.Operation to control their output.

Reported by: loic84 Owned by: Loic Bistuer <loic.bistuer@…>
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently MigrationWriter controls the output of migrations and to do so it renders the various operations with a generic format.

The problem is operations such as CreateModel end up writing all the fields on a single line, which makes the migration file difficult to review.

I propose that Operations should control their own output.

I had a go at this in https://github.com/django/django/pull/1681.

Attachments (0)

Change History (6)

comment:1 Changed 6 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 months ago by andrewgodwin

Me and loic84 discussed this on IRC a while back; I'm not a fan of how this currently looks (it makes the operations very hard to read and is confusing logic with presentation in a way) but it's a nice idea in principle.

comment:3 Changed 5 months ago by loic84

I've cleaned up the code a bit by introducing a MigrationFormatter class, but ultimately there isn't so much we can do; presentational logic in Python is quite verbose. Also the way the serializer is implemented requires formatting as we go, fixing that would require a significant refactor.

If we move to a better template system with white spaces handling (Jinja2), CreateModel.serialize() could be shrunk significantly.

IMO being able to review the output of CreateModel is sufficiently critical to justify an imperfect CreateModel.serialize() method.

comment:4 Changed 5 months ago by loic84

I've reworked the code in a way that doesn't affect the Operation classes anymore, the changes are now contained in migrations/writer.py.

comment:5 Changed 3 months ago by Loic Bistuer <loic.bistuer@…>

  • Owner set to Loic Bistuer <loic.bistuer@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 374faa472150e2e1fb70224a764b82d3d51994f7:

Fixed #21323 -- Improved readability of serialized Operation.

comment:6 Changed 3 months ago by Andrew Godwin <andrew@…>

In e8d4aed3b97493a050f69dca1f6562991d5c511b:

Merge pull request #1681 from loic/migrations.format

Fixed #21323 -- Improved readability of serialized Operation.

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.