Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#22791 closed Cleanup/optimization (fixed)

makemigrations interactive questioner ignores app labels

Reported by: Ben Davis Owned by: Huu Nguyen
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Ben Davis Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When running makemigrations <app_label>, the interactive questioner will still ask questions regarding apps you didn't pass to the command, even though the migrations aren't created for those apps.

Change History (11)

comment:1 Changed 2 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Hi,

Indeed, I've managed to reproduce the behavior you describe.

I agree that we should try and make the interactive a bit smarter if we can (ie, not ask questions that won't be used for creating migrations).

Thanks.

comment:2 Changed 2 years ago by Huu Nguyen

Owner: changed from nobody to Huu Nguyen
Status: newassigned

comment:3 Changed 2 years ago by Huu Nguyen

I'd actually change this ticket to, "makemigrations ignores app labels for conflicts". From the looks of it, there are three issues I would classify as improper behavior:

  1. (Original ticket) Interactive questioner will ask a "y/N" question for an app that was not specified.
  2. An outstanding conflict in an unspecified app will throw a CommandErorr if merge is set to False.
  3. An outstanding conflict in an unspecified app will create a migration file if merge is set to True, interactive is set to True, and "y" is answered.

The expected behavior is to ignore apps that have not been specified when at least one app has been specified. There are two ways of going about eliminating these behaviors:

  1. Filter out conflicts from loader.detect_conflicts in the makemigrations module by app_labels. This localizes the required changes but may be inefficient as conflicts will be scanned for unspecified apps.
  2. Pass in app_labels to loader.detect_conflicts() and only scan for conflicts in the apps that have been specified. This is more efficient but touches more code.

I'd like to hear what people have to say about this. If necessary, I can create the tickets for issues 2 and 3 if merging them all into this one is inappropriate.

Last edited 2 years ago by Huu Nguyen (previous) (diff)

comment:4 Changed 2 years ago by Huu Nguyen

Made a quick change in makemigrations using approach 1.

PR sent: ​​https://github.com/django/django/pull/2834

comment:5 Changed 2 years ago by Huu Nguyen

Has patch: set

comment:6 Changed 2 years ago by Tim Graham

Patch needs improvement: set

I left comments for improvement on PR. Please uncheck "Patch needs improvement" when you update it, thanks.

comment:7 Changed 2 years ago by Huu Nguyen

Patch needs improvement: unset

The PR has been updated using suggestions left by Tim.

comment:8 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Looks good to me. Bumping to RFC to give Andrew a chance to look and ensure he's okay with the approach rather than the other option you mentioned.

comment:9 Changed 2 years ago by Ben Davis

Cc: Ben Davis added

comment:10 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In f7a78f9bba4efd0231aec0326a73fdbd004d1faa:

Fixed #22791 -- Invoke interactive questioner only for conflicts in specified apps.

Thanks bendavis78 for the report and Tim Graham for the review.

comment:11 Changed 2 years ago by Tim Graham <timograham@…>

In 6d5238f6c873f8ad652ee206b88849a50e68accc:

[1.7.x] Fixed #22791 -- Invoke interactive questioner only for conflicts in specified apps.

Thanks bendavis78 for the report and Tim Graham for the review.

Backport of f7a78f9bba from master

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