Opened 4 weeks ago
Closed 11 hours ago
#36103 closed Cleanup/optimization (duplicate)
Simplify get_namespace() and adjust get_and_report_namespace() method signature for automatic imports in Shell command
Reported by: | Salvo Polizzi | Owned by: | Salvo Polizzi |
---|---|---|---|
Component: | Core (Management commands) | Version: | 5.2 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Pull Requests: | 19079 build:success | ||
Description (last modified by ) ¶
Following the last discussion in PR, we propose the following improvements to enhance readability and reduce confusion:
- Simplify
get_namespace()
:- The current implementation of get_namespace() can be rewritten as a one-liner for better readability without changing its functionality.
- Example:
#current def get_namespace(self): apps_models = apps.get_models() namespace = {} for model in reversed(apps_models): if model.__module__: namespace[model.__name__] = model return namespace #proposed def get_namespace(): return {m.__name__: m for m in reversed(apps.get_models()) if m.__module__}
- Update
get_and_report_namespace()
:- The
no_imports
parameter in the method signature can be misleading and cause confusion for its users. - Proposed Change: Remove the
no_imports
parameter from the method and instead handle its logic in the relevant runners where the namespace is imported. - Justification: This improves separation of concerns and makes the method more focused.
- The
Change History (8)
comment:1 by , 4 weeks ago
Description: | modified (diff) |
---|
follow-up: 4 comment:2 by , 4 weeks ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | dev → 5.2 |
comment:3 by , 4 weeks ago
Summary: | Simplify get_namespace() and adjust get_and_report_namespace() method signature → Simplify get_namespace() and adjust get_and_report_namespace() method signature for automatic imports in Shell command |
---|
comment:4 by , 4 weeks ago
Replying to Natalia Bidart:
Thank you Salvo for this ticket, I overall agree except for the
no_imports
, I have no problem to remove it fromget_and_report_namespace
but we should do it in a way that does not duplicate logic/checks. Thank you!
Maybe we can wait until someone has other better ideas for the no_imports
and in the meantime stick with this implementation, e.g. leave it as a parameter.
comment:5 by , 3 weeks ago
Has patch: | set |
---|
follow-up: 7 comment:6 by , 14 hours ago
Patch needs improvement: | set |
---|
Hi Salvo, if you could resolve the merge conflicts that would help us assess next steps. Thanks.
comment:7 by , 14 hours ago
Replying to Jacob Walls:
Hi Salvo, if you could resolve the merge conflicts that would help us assess next steps. Thanks.
Hi, after reviewing Natalia's patch (https://github.com/django/django/pull/19147), I believe this ticket might no longer be necessary. Could you please let me know if I should close the PR or if further action is needed? Thanks!
comment:8 by , 11 hours ago
Resolution: | → duplicate |
---|---|
Status: | assigned → closed |
I think the PR can be closed. Appreciate the confirmation.
Not sure of the best resolution to approximate "overcome by events" but I'll choose duplicate for now? This cleanup was mooted by #36158.
Thank you Salvo for this ticket, I overall agree except for the
no_imports
, I have no problem to remove it fromget_and_report_namespace
but we should do it in a way that does not duplicate logic/checks. Thank you!