-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
Fixed #36533 -- Fix startapp to allow empty directories as valid targets. #19708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2303e27
to
ce0d271
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ⛵️!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR 👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 still true after the recent changes. What I would expect is:
- doing
django-admin startapp example os
ormanage.py startapp example os
should both be disallowed, even when there is an emptyos
directory in the project - doing
django-admin startapp example destination
ormanage.py startapp example destination
should be allowed, creating the app indestination
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get this to ever quite work right. I think the core issue is with find_spec()
treating an empty directory as an importable module. And since django-admin
runs outside of any specific project it doesn't see it and allows the creation if it already exists but manage.py
detects the empty directory and thinks its a module.
So i either have to run find_spec and its thinks an empty directory is a module or ignore the directory which skips the validation. Neither of which are good solutions because they cause a different problem with the command.
703a4e7
to
d8ccaef
Compare
95e6a03
to
6be5f1f
Compare
6be5f1f
to
5b05337
Compare
5b05337
to
ddf1a87
Compare
Trac ticket number
ticket-36533
Branch description
manage.py startapp <name> <directory>
incorrectly raised aCommandError
when the target directory already existed, even if it was empty and had a valid name.This prevented valid workflows such as creating an app inside an intentionally pre-created directory (e.g.
manage.py startapp blog apps/blog
).Changes:
Updated
startapp
handling to:CommandError
if the directory is non-empty (to avoid silent overwrites).os
remain disallowed).os
is disallowed as a fresh target, but an emptydestination/
directory is valid).Unified behavior of
django-admin startapp
andmanage.py startapp
.Tests:
destination
) do not raise false errors.os
(a Python module) is always disallowed.Expected Behavior:
django-admin startapp example os
-> disallowedmanage.py startapp example os
-> disalloweddjango-admin startapp example destination
-> allowed (even ifdestination/
exists and is empty)manage.py startapp example destination
-> allowed (even ifdestination/
exists and is empty)Checklist
main
branch.