#32285 closed Cleanup/optimization (fixed)
AppConfig.label should raise an exception when it's not a valid Python identifier.
Reported by: | Federico Capoano | Owned by: | Hasan Ramezani |
---|---|---|---|
Component: | Core (Other) | Version: | 3.1 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
Follow up to mailing list discussion.
Setting the app_label
of an AppConfig
object is allowed and fails later on during migrations (./manage.py migrate
) with (too many values to unpack)
.
It took me a couple of minutes to figure out what was wrong, but it may not be so obvious to an inexperienced user so it may be better to raise a clearer exception.
Maybe we can use system checks and fail earlier if such a problem is detected?
Change History (11)
comment:1 by , 4 years ago
Component: | Core (System checks) → Core (Other) |
---|---|
Resolution: | → needsinfo |
Status: | new → closed |
Summary: | AppConfig: app_label with dot should raise exception → AppConfig.label should raise an exception when it's not a valid Python identifier. |
Type: | Bug → Cleanup/optimization |
comment:2 by , 4 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
It looks that it will be good to raise an error to avoid confusions (see #32287), even if I cannot reproduce this crash.
comment:3 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 5 comment:4 by , 4 years ago
Have you already tried to create a new django app, put a dot in the app_label field of the AppConfig, ensure the AppConfig is mentioned in the init.py, then create a model and run ./manage.py makemigrations?
comment:5 by , 4 years ago
Replying to Federico Capoano:
Have you already tried to create a new django app, put a dot in the app_label field of the AppConfig, ensure the AppConfig is mentioned in the init.py, then create a model and run ./manage.py makemigrations?
Yes. Can you share a stacktrace? I'm not even sure in which line or file you encountered this issue 🤔
comment:7 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:8 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:11 by , 3 years ago
I was surprised by this error when trying to upgrade a large codebase to 3.2. This codebase's convention is to use app labels with dashes, which I suspect still works fine (unlike labels with periods).
Is there a recommended migration path for projects like these that were using an invalid label unknowingly? As far as I can tell, the only option is to change the label, which looks involved: it affects things like old migrations, db rows like content types, and maybe even table names.
If there's no simple migration path, what about changing this from an exception to a warning? That'd still accomplish the goal of informing users that something may break and allows cases like mine to avoid quite a bit of pain.
Raising
ImproperlyConfigured
should be enough, however everything works for me with dotted label, e.g.ticket_32285.test_one
. Can you provide a sample project?