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 Salvo Polizzi)

Following the last discussion in PR, we propose the following improvements to enhance readability and reduce confusion:

  1. 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__}
      
  1. 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.

Change History (8)

comment:1 by Salvo Polizzi, 4 weeks ago

Description: modified (diff)

comment:2 by Natalia Bidart, 4 weeks ago

Triage Stage: UnreviewedAccepted
Version: dev5.2

Thank you Salvo for this ticket, I overall agree except for the no_imports, I have no problem to remove it from get_and_report_namespace but we should do it in a way that does not duplicate logic/checks. Thank you!

comment:3 by Natalia Bidart, 4 weeks ago

Summary: Simplify get_namespace() and adjust get_and_report_namespace() method signatureSimplify get_namespace() and adjust get_and_report_namespace() method signature for automatic imports in Shell command

in reply to:  2 comment:4 by Salvo Polizzi, 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 from get_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 Salvo Polizzi, 3 weeks ago

Has patch: set

comment:6 by Jacob Walls, 14 hours ago

Patch needs improvement: set

Hi Salvo, if you could resolve the merge conflicts that would help us assess next steps. Thanks.

in reply to:  6 comment:7 by Salvo Polizzi, 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 Jacob Walls, 11 hours ago

Resolution: duplicate
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top