Django

Code

Ticket #6017 (closed: fixed)

Opened 1 year ago

Last modified 5 months ago

order of django-admin.py options matters, but shouldn't

Reported by: toddobryan Assigned to: nobody
Milestone: Component: Core framework
Version: SVN Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description (Last modified by russellm)

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

lax-option-parser.patch (2.9 kB) - added by toddobryan on 11/22/07 21:07:56.
6017-r6718.diff (4.3 kB) - added by russellm on 11/26/07 07:08:37.
Revised patch for lax option parser; comment and test case changes.
django-admin-tests.diff (38.1 kB) - added by russellm on 07/07/08 08:43:31.
Prototype tests for django-admin and manage.py behaviour
django-admin-tests-v2.diff (40.3 kB) - added by russellm on 07/08/08 09:46:46.
Updated, Scriptless version of the prototype django-admin tests
6017-r7877.diff (10.3 kB) - added by russellm on 07/10/08 09:04:27.
Updated patch with test cases using new django-admin test framework

Change History

11/22/07 21:07:56 changed by toddobryan

  • attachment lax-option-parser.patch added.

11/22/07 21:12:43 changed by toddobryan

  • needs_better_patch changed.
  • has_patch set to 1.
  • component changed from Uncategorized to Core framework.
  • needs_tests changed.
  • needs_docs changed.

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.

11/26/07 07:07:45 changed by russellm

  • description changed.
  • needs_tests set to 1.
  • stage changed from Unreviewed to 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.

11/26/07 07:08:37 changed by russellm

  • attachment 6017-r6718.diff added.

Revised patch for lax option parser; comment and test case changes.

12/02/07 02:16:06 changed by shaleh

  • needs_tests deleted.

Patch has tests.

12/02/07 03:29:34 changed by Simon G <dev@simon.net.nz>

  • stage changed from Accepted to Ready for checkin.

07/07/08 08:43:31 changed by russellm

  • attachment django-admin-tests.diff added.

Prototype tests for django-admin and manage.py behaviour

07/08/08 09:46:46 changed by russellm

  • attachment django-admin-tests-v2.diff added.

Updated, Scriptless version of the prototype django-admin tests

07/10/08 07:15:14 changed by russellm

(In [7876]) Refs #5943, #6017 -- Added framework and tests to check for the correct operation of django-admin and manage.py. Thanks for Simon Willison and Karen Tracey for their contributions and assistance testing this patch.

07/10/08 09:04:27 changed by russellm

  • attachment 6017-r7877.diff added.

Updated patch with test cases using new django-admin test framework

07/10/08 09:14:08 changed by russellm

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.

07/11/08 08:18:20 changed by russellm

  • status changed from new to closed.
  • resolution set to fixed.

(In [7888]) Fixed #6017 -- Modified the Lax parser to allow --settings and the other core management arguments to appear anywhere in the argument list. Thanks to Todd O'Bryan for the suggestion and patch.


Add/Change #6017 (order of django-admin.py options matters, but shouldn't)




Change Properties
Action