Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#30300 closed New feature (wontfix)

Allow migrations directories without __init__.py files

Reported by: Benjy Weinberger Owned by: Benjy Weinberger
Component: Database layer (models, ORM) Version: 3.1
Severity: Release blocker Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

Background: In python 3 a package with no __init__.py is implicitly a namespace package, so it has no __file__ attribute.

The migrate command currently checks for existence of a __file__ attribute on the migrations package. This check was introduced in #21015, because the __file__ attribute was used in migration file discovery.

However, in #23406 migration file discovery was changed to use pkgutil.iter_modules (), instead of direct filesystem access. pkgutil. iter_modules() uses the package's __path__ list, which exists on implicit namespace packages.

As a result, the __file__ check is no longer needed, and in fact prevents migrate from working on namespace packages (implicit or otherwise).

Related work: #29091

Change History (16)

comment:1 by Benjy Weinberger, 5 years ago

Owner: changed from nobody to Benjy Weinberger
Status: newassigned

comment:3 by Carlton Gibson, 5 years ago

Triage Stage: UnreviewedAccepted
Type: BugNew feature

Hi Benjy. Thanks for the report — and the test case in the PR! Unless something comes up in review, this seems reasonable to me.

I'm going to call it a new feature, since this behaviour will never have worked I think.

... (implicit or otherwise)

I don't quite follow the or otherwise there?

comment:4 by Carlton Gibson, 5 years ago

Summary: migrate silently ignores apps whose migrations directory doesn't have an __init__.pyAllow migrations directories without __init__.py files.

comment:5 by Tim Graham, 5 years ago

Description: modified (diff)
Summary: Allow migrations directories without __init__.py files.Allow migrations directories without __init__.py files

What's the use case? When makemigrations generates the first migration, it adds an __init__.py file. There was some concern about encouraging namespace packages in ticket:23919#comment:102.

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:6 by Benjy Weinberger, 5 years ago

The "or otherwise" refers to namespace packages being either implicit (lacking an __init__.py) or explicit (an __init__.py containing an old-style namespace package incantation).

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:7 by Benjy Weinberger, 5 years ago

Re the use case:

It seems unnecessary to insist on an empty __init__.py just to preserve a (stale) check for __file__, when they are otherwise not required in Python 3.

And #29091 implies that this should be allowed.

Context is: We've just finished migrating our codebase to all-python 3, and nuked all our __init__py files. We are now upgrading to Django 2.1, and noticing the various places where they're still de-facto required by Django. This particular case seemed unnecessary and easy to fix.

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:8 by Václav Řehák, 5 years ago

We had a similar issue with test discovery. One of our developers read an article that __init__.py files are not required on Python3 and started removing them. Everything seemingly worked but some tests were not discovered and did not run in CI (which made it difficult to spot).

I think Django should not required them if technically possible or at least it should be made clear in the documentation (sorry if I overlooked it).

comment:9 by Benjy Weinberger, 5 years ago

Friendly nudge?

comment:10 by Mariusz Felisiak, 5 years ago

Has patch: set
Version: 2.1master

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 3cd3bebe:

Fixed #30300 -- Allowed migrations to be loaded from directories without init.py file.

comment:12 by Tim Graham, 4 years ago

I raised this issue on the mailing list as there was a thread that concluded that Django shouldn't promote use of implicit namespace packages.

comment:13 by Tim Graham, 4 years ago

Resolution: fixed
Severity: NormalRelease blocker
Status: closednew
Version: master3.1

Thus far there haven't been any mailing list replies in favor this change. I created a PR to revert it and modify the test accordingly.

comment:14 by GitHub <noreply@…>, 4 years ago

Resolution: fixed
Status: newclosed

In ff55adbd:

Reverted "Fixed #30300 -- Allowed migrations to be loaded from directories without init.py file."

This reverts commit 3cd3bebe8921e14b911b36b2a1cbceef8fb6294e.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d004bce:

[3.1.x] Reverted "Fixed #30300 -- Allowed migrations to be loaded from directories without init.py file."

This reverts commit 3cd3bebe8921e14b911b36b2a1cbceef8fb6294e.
Backport of ff55adbd0da6618abaf265d16196bf54f81aa77a from master

comment:16 by Mariusz Felisiak, 4 years ago

Resolution: fixedwontfix
Triage Stage: AcceptedUnreviewed
Note: See TracTickets for help on using tickets.
Back to Top