Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#22791 closed Cleanup/optimization (fixed)

makemigrations interactive questioner ignores app labels

Reported by: bendavis78 Owned by: whoshuu
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: bendavis78 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 11 months ago by bmispelon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to 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 Changed 10 months ago by whoshuu

  • Owner changed from nobody to whoshuu
  • Status changed from new to assigned

comment:3 Changed 10 months ago by whoshuu

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 months ago by whoshuu (previous) (diff)

comment:4 Changed 10 months ago by whoshuu

Made a quick change in makemigrations using approach 1.

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

comment:5 Changed 10 months ago by whoshuu

  • Has patch set

comment:6 Changed 10 months ago by timo

  • Patch needs improvement set

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

comment:7 Changed 10 months ago by whoshuu

  • Patch needs improvement unset

The PR has been updated using suggestions left by Tim.

comment:8 Changed 10 months ago by timo

  • Triage Stage changed from Accepted to 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 Changed 10 months ago by bendavis78

  • Cc bendavis78 added

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

  • Resolution set to fixed
  • Status changed from assigned to 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 Changed 10 months 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