Opened 17 years ago
Closed 16 years ago
#6017 closed (fixed)
order of django-admin.py options matters, but shouldn't
Reported by: | Todd O'Bryan | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | 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 (last modified by )
In ticket #5369, django-admin.py
and
manage.py
are supposed to read all options, looking for
--settings
and
--pythonpath
, and then use those two if they're present to access app-supplied commands. The code (which was mine) has a bug, in that options stop getting read as soon as something other than
--settings
or
--pythonpath
gets hit.
Without the patch, you currently get:
>>> from django.core.management import LaxOptionParser, get_version >>> from django.core.management.base import BaseCommand >>> parser = LaxOptionParser(version=get_version(), option_list=BaseCommand.option_list) >>> options, args = parser.parse_args('django-admin.py command --option --settings=settingsmodule --pythonpath=/home/user/django-dir'.split()) >>> print options {'pythonpath': None, 'settings': None} >>> args ['django-admin.py', 'command', '--settings=settingsmodule', '--pythonpath=/home/user/django-dir']
where both --pythonpath
and
--settings
get ignored.
With the included patch, you get:
>>> from django.core.management import LaxOptionParser, get_version >>> from django.core.management.base import BaseCommand >>> parser = LaxOptionParser(version=get_version(), option_list=BaseCommand.option_list) >>> options, args = parser.parse_args('django-admin.py command --option --settings=settingsmodule --pythonpath=/home/user/django-dir'.split()) >>> print options {'pythonpath': '/home/user/django-dir', 'settings': 'settingsmodule'} >>> args ['django-admin.py', 'command', '--option']
as desired.
If someone wants me to stick the doctest code somewhere, please tell me where. I think I may need to make a whole new folder, but I don't want to make a mess.
Attachments (5)
Change History (12)
by , 17 years ago
Attachment: | lax-option-parser.patch added |
---|
comment:1 by , 17 years ago
Component: | Uncategorized → Core framework |
---|---|
Has patch: | set |
comment:2 by , 17 years ago
Description: | modified (diff) |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
The patch itself looks good; my quick poking didn't reveal any problems, but that's just quick poking. Since you worked out a way to regression test this bit, it should be a bit more comprehensive than one test. There are several branch paths in the patch - these should all be tested (at the very least, -X , --X and --X=Y option types should all be checked, in various orders and combinations with --settings etc).
Also, a minor point of style - calling the test directory 'management' is sufficient.
I've attached a slightly revised patch, with a couple more tests and cleaned up comments; if you can bulk out the patch with some more tests, I'll commit.
by , 17 years ago
Attachment: | 6017-r6718.diff added |
---|
Revised patch for lax option parser; comment and test case changes.
comment:4 by , 17 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
by , 16 years ago
Attachment: | django-admin-tests.diff added |
---|
Prototype tests for django-admin and manage.py behaviour
by , 16 years ago
Attachment: | django-admin-tests-v2.diff added |
---|
Updated, Scriptless version of the prototype django-admin tests
comment:5 by , 16 years ago
by , 16 years ago
Attachment: | 6017-r7877.diff added |
---|
Updated patch with test cases using new django-admin test framework
comment:6 by , 16 years ago
This patch is close - probably close enough that I would consider committing it as is. There is one niggling problem: if you use --help on a command, you get the help for the core options output twice. If you read the output, it is showing two possible usages - ./manage.py --help, and ./manage.py command --help. This is reasonable, but it might be nice to prevent the './manage.py --help' case getting displayed when the user is explicitly asking for command specific help.
If I can't think of a way to fix this in the next few days, I'll probably commit as is - the bug (such as it is) is present in the current trunk, so it's not being introduced by this patch, and it would be good to dispatch the bug that _is_ fixed by this patch. However, I'll wait until I'm not so sleep deprived before I make that call.
comment:7 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I so wish we could edit tickets after we make a mess of them. (Or maybe I should learn to click the preview button.)
Anyway, I included tests in
django/tests/regressiontests/core_management/tests.py
. I also included empty
init.py
and
models.py
files, but I don't think those show up.