Fixed #36533 -- Refactored startapp handle method to simplify empty directory handling. #19708
Conversation
2303e27 to
ce0d271
Compare
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
tests/admin_scripts/tests.py
Outdated
| def test_invalid_target_name(self): | ||
| for bad_target in ( | ||
| "invalid.dir_name", | ||
| "7invalid_dir_name", | ||
| ".invalid_dir_name", | ||
| ): | ||
| with self.subTest(bad_target): | ||
| _, err = self.run_django_admin(["startapp", "app", bad_target]) | ||
| self.assertOutput( | ||
| err, | ||
| "CommandError: '%s' is not a valid app directory. Please " | ||
| "make sure the directory is a valid identifier." % bad_target, | ||
| ) | ||
|
|
||
| def test_importable_target_name(self): | ||
| _, err = self.run_django_admin(["startapp", "app", "os"]) | ||
| self.assertOutput( | ||
| err, | ||
| "CommandError: 'os' conflicts with the name of an existing Python " | ||
| "module and cannot be used as an app directory. Please try " | ||
| "another directory.", | ||
| ) |
There was a problem hiding this comment.
Note that django-admin startapp example os results in CommandError: 'os' conflicts with the name of an existing Python module and cannot be used as an app directory. Please try another directory.
But after this change it is allowed.
This is reverting much of ticket-30393, which is done because if you were to then add the app created in the os directory, this would break the project with ModuleNotFoundError: No module named 'os.apps'; 'os' is not a package
There was a problem hiding this comment.
Thanks for this! I've had a look more closely and the issue might be how manage.py and django-admin handles import checks. I think manage.py startapp causes a false CommandError if the target directory exists and sys.path thinks it's an importable module even if it is empty.
I'll change the patch to adjust this behaviour and correct the changes to the tests.
af61f6c to
703a4e7
Compare
…irectory handling.
703a4e7 to
d8ccaef
Compare
Trac ticket number
ticket-36533
Branch description
This PR resolves an inconsistency between
manage.py startappanddjango-admin startappregarding validation of the destination directoy name.manage.py startapp <app_name> <directory>would incorrectly raiseCommandErrorwhen the destination directory name matched an importable Python module (eg.destination,os) even if theapp_nameitself was valid.This behaviour differs from
django-adminwhich allows this usage.Changes:
startappto only validate the app name against Python modules aligning the functionality ofmanage.py startappanddjango-admin startappNow
manage.py startappanddjango-admin startappshould have the same behaviour, and false positiveCommandError's wont occur when when destination directories have coincidental names with Python modules. I realised aswell that something likemanage.py startapp blog apps/blogis now valid and would have errored before.Checklist
mainbranch.