#33917 closed Cleanup/optimization (wontfix)
Optimize ModelBase.__new__ app_label setup
Reported by: | Ivan Dominic Baguio | Owned by: | Ivan Dominic Baguio |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.0 |
Severity: | Normal | Keywords: | ModelBase |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
While browsing the ModelBase code, i noticed a possible improvement on the code logic.
under ModelBase.__new__
, you will see:
# Look for an application configuration to attach the model to. app_config = apps.get_containing_app_config(module) if getattr(meta, "app_label", None) is None: if app_config is None: # ... else: app_label = app_config.label
notice that app_config
is defined above and outside the if getattr
block.
the app_config
variable is only used inside the if getattr
block.
it would make sense to move the app_config
initialization inside the if
block, so it doesn't get unnecessarily called.
i've double checked the following:
get_containing_app_config
doesn't have any side effects, and is only a getter methodapp_config
is only needed inside the if block
Attachments (1)
Change History (4)
by , 2 years ago
Attachment: | 0001-Move-app_config-initialization-inside-if-block.patch added |
---|
comment:1 by , 2 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Summary: | Improve ModelBase.__new__ app_label setup → Optimize ModelBase.__new__ app_label setup |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 2 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
Triage Stage: | Accepted → Unreviewed |
apps.get_containing_app_config()
has a side-effect, it calls check_apps_ready()
that raises an exception if all apps haven't been imported yet. IMO we shouldn't change this.
Sounds reasonable. For the future, small cleanups don't need tickets.
PR