Opened 2 years ago

Closed 2 months 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: master
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 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

Okay, let's see what it looks like.

comment:2 Changed 2 years ago by Akshesh Doshi

Has patch: set
Owner: changed from nobody to Akshesh Doshi
Status: newassigned

comment:3 Changed 2 years ago by Tim Graham

Has patch: unset
Triage Stage: AcceptedSomeday/Maybe

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

comment:4 Changed 2 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

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