Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22791 closed Cleanup/optimization (fixed)

makemigrations interactive questioner ignores app labels

Reported by: Ben Davis Owned by: Huu Nguyen
Component: Migrations Version: dev
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 by Baptiste Mispelon, 10 years ago

Triage Stage: Unreviewed → Accepted
Type: Uncategorized → Cleanup/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 by Huu Nguyen, 10 years ago

Owner: changed from nobody to Huu Nguyen
Status: new → assigned

comment:3 by Huu Nguyen, 10 years ago

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 10 years ago by Huu Nguyen (previous) (diff)

comment:4 by Huu Nguyen, 10 years ago

Made a quick change in makemigrations using approach 1.

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

comment:5 by Huu Nguyen, 10 years ago

Has patch: set

comment:6 by Tim Graham, 10 years ago

Patch needs improvement: set

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

comment:7 by Huu Nguyen, 10 years ago

Patch needs improvement: unset

The PR has been updated using suggestions left by Tim.

comment:8 by Tim Graham, 10 years ago

Triage Stage: Accepted → Ready 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 by Ben Davis, 10 years ago

Cc: Ben Davis added

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

Resolution: → fixed
Status: assigned → closed

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 by Tim Graham <timograham@…>, 10 years ago

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