Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#21630 closed Cleanup/optimization (fixed)

Simplify find_management_module

Reported by: Aymeric Augustin Owned by: Claude Paroz <claude@…>
Component: Core (Management commands) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There's a comment that says: "When using manage.py, the project module is added to the path, loaded, then removed from the path."

I believe manage.py no longer does that since 1.4 (new project layout). As a consequence the code could be simplified.

Attachments (1)

21630.diff (2.9 KB) - added by Claude Paroz 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by Ramiro Morales

Thought the same when trying to work on #15084 and reviewing that code.

But, shouldn't that code remain as is to stay compatible with existing project that use old the layout? Or this isn't a supported scenario?

comment:2 Changed 3 years ago by Marc Tamlyn

I think we've had the new layout long enough - other things which tried to do clever stuff around this have been removed from what I remember. +1 to removing this.

comment:3 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

Changed 3 years ago by Claude Paroz

Attachment: 21630.diff added

comment:4 Changed 3 years ago by Claude Paroz

Has patch: set

Is this too much simplification?

comment:5 Changed 2 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:6 Changed 2 years ago by Aymeric Augustin

Owner: Aymeric Augustin deleted
Status: assignednew
Triage Stage: AcceptedReady for checkin

Yes, your approach seems correct to me. I'd rather remove this stuff and see if someone has an edge case that breaks than leave it without knowing why. The patch will require a small update to apply after 486532. I'll let you push it!

comment:7 in reply to:  1 Changed 2 years ago by Aymeric Augustin

Replying to ramiro:

But, shouldn't that code remain as is to stay compatible with existing project that use old the layout? Or this isn't a supported scenario?

Because of the app-loading refactor, I don't think it'll be possible to use the old layout with Django 1.7.

comment:8 Changed 2 years ago by Claude Paroz <claude@…>

Owner: set to Claude Paroz <claude@…>
Resolution: fixed
Status: newclosed

In fe53bc524b9c7cbc814868acde94226d0ba2bc84:

Fixed #21630 -- Simplified management command discovery

Thanks Aymeric Augustin for the report end the review.

comment:9 Changed 2 years ago by Claude Paroz

I'm not convinced this has to be backported to 1.7. If you think it should, just speak!

comment:10 Changed 2 years ago by Aymeric Augustin

Yes, I think it should be backported, because if it breaks something, that will be seen as collateral damage of app loading.

comment:11 Changed 2 years ago by Claude Paroz <claude@…>

In 91ef348bd6ac5d3051d428806b143c419af91aa8:

[1.7.x] Fixed #21630 -- Simplified management command discovery

Thanks Aymeric Augustin for the report end the review.
Backport of fe53bc524 from master.

comment:12 Changed 2 years ago by Aymeric Augustin

Thank you!

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