Opened 8 years ago

Closed 6 years ago

#26906 closed Cleanup/optimization (fixed)

Factor out an AlterFooTogether operation from AlterUnique/IndexTogether

Reported by: Akshesh Doshi Owned by: Akshesh Doshi
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The reasons why I think this should be done are

1) When a bug is reported for any one of them (like #26180) the owner or reviewer has to check if the bug also exists (most of the time it does) for the other operation and apply the same fix for the other (see ​!6926), or else the fix might get delayed until another ticket is opened reporting the same issue for the other operation.

2) Recently I realized that a small part of AlterIndexTogether code is untested but tests exist for those lines for AlterUniqueTogether so having the same source of code for both the operations will result in better code coverage!

3) More than 90% of the code for both the operations is the same so in honor of the DRY rule (this is maybe kinda the reason for point 1 :P).

Change History (4)

comment:1 by Tim Graham, 8 years ago

Triage Stage: Unreviewed → Accepted

Okay, let's see what it looks like.

comment:2 by Akshesh Doshi, 8 years ago

Has patch: set
Owner: changed from nobody to Akshesh Doshi
Status: new → assigned

PR: ​https://github.com/django/django/pull/6931

comment:3 by Tim Graham, 8 years ago

Has patch: unset
Triage Stage: Accepted → Someday/Maybe

From the pull request, "we should probably redesign the Alter[Index|Unique]Together] classes before tackling this PR here."

comment:4 by Tim Graham <timograham@…>, 6 years ago

Resolution: → fixed
Status: assigned → closed

In fc16015d:

Fixed #26906 -- Reduced alter together operations code duplication.

Thanks Akshesh Doshi for the initial patch.

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