Opened 5 years ago

Closed 5 years ago

#30098 closed New feature (wontfix)

Permit using packages (directories) in custom django-admin commands.

Reported by: Andrey Volkov Owned by: Andrey Volkov
Component: Core (Management commands) Version: 2.1
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Current Django behavior is that only files are allowed for custom django-admin commands definition. It would be great if Django permitted directories (see example below).

Current (required) style:

app_example/management/commands
├── first_command.py
└── second_command.py

New style:

app_example/management/commands
├── first_command
│   ├── __init__.py
│   └── some_useful_data
└── second_command
    └── __init__.py

Or combined style:

app_example/management/commands
├── first_command
│   ├── __init__.py
│   └── some_useful_data
└── second_command.py

This feature allows developers to use additional files (some_useful_data is this case) in structured way, also providing encapsulation.

The only code to be changed is in file django/core/management/__init__.py line 27:
From if not is_pkg and not name.startswith('_')]
To if not name.startswith('_')]

After this change, function find_commands won't ignore packages (directories).

Also, backward compatibility will be saved (because single files work in the same way).

Change History (8)

comment:1 by Andrey Volkov, 5 years ago

Now I am working on tests and documentation.

comment:2 by Tim Graham, 5 years ago

At first glance, I'm not in favor of the extra complexity this would introduce. Django stores helper files for its own commands in the management directory (django/core/managment) which seems fine.

in reply to:  2 comment:3 by Andrey Volkov, 5 years ago

Replying to Tim Graham:

At first glance, I'm not in favor of the extra complexity this would introduce. Django stores helper files for its own commands in the management directory (django/core/managment) which seems fine.

Yeah, this can increase complexity, but this feature is not obligatory. Developers can write management commands the same way as before.

Why I want this change, is because it solves and/or simplifies some problems (mine too). For example, we could have a library that assumes some filenames to be data.json and we want to use this library in the number of commands. With directories style it can be solved like this:

app_example/management/commands
├── first_command
│   ├── data.json
│   └── __init__.py
└── second_command
    ├── data.json
    └── __init__.py

With files-only style it (with some changes to library behavior) should be:

app_example/management
├── commands
│   ├── first_command.py
│   └── second_command.py
├── first_command_data.json
└── second_command_data.json

Last solution gives duplicated command names. That means if developer changes a command name, it should be changed at least twice (against one change in directory approach).

Also, this style gives possibility for encapsulation. Django wouldn't know how the command is structured (is it file or directory) it will just use it as a regular module, that contains a Command class.

Last point I want to mention here, is that it can help Django in the future. If additional functionality will be given to the management module (some features except commands), encapsulation will prevent storing random files (that refer only to commands) in management directory. And change, that enables directory-based commands, is extremely small, so that it affects only parsing the app/management/commands directory (it is literally removes only one condition).

Sorry for such a long comment, but I really think that this feature will help developers both now and in the future.

comment:4 by Andrey Volkov, 5 years ago

Patch is finished. Pull request is submitted. https://github.com/django/django/pull/10836

comment:5 by Andrey Volkov, 5 years ago

Has patch: set

comment:6 by Andrey Volkov, 5 years ago

Patch needs improvement: set

Documentation fails its build, because django is considered as misspelling.

comment:7 by Andrey Volkov, 5 years ago

Patch needs improvement: unset

Fixed documentation. All checks have passed.

comment:8 by Carlton Gibson, 5 years ago

Resolution: wontfix
Status: assignedclosed

Hi Andrey. Thanks for the suggestion and PR demonstrating it. As it stands I think I agree with Tim's initial comment.

I'm not sure I see any real benefit to allowing packages here. It's already possible to add a package to contain any supporting files (if there's more than one) — either in management or in commands — so I'm not convinced any extra encapsulation merits the change. (I'm not sure I buy either the renaming or extra features points you make.)

The restriction to modules has been there ≈forever. See f25b8cdbcdc99812299e9081cd3b03ddc08dcf74 for #5222, which introduced find_commands(). Just on the principle that There should be one-- and preferably only one --obvious way to do it. I don't think changing that design choice is merited.

Also, people will be using the fact that you can put a package in commands and not have it detected. If we change that such packages will be picked up by find_commands() and we'll have a whole load of reports of help listing phantom commands that only lead to AttributeError: module '...' has no attribute 'Command' errors when invoked.

So for both these reasons I'm going to opt for wontfix here. Possibly you could raise it on the DevelopersMailingList if you feel strongly on the issue.

Thanks again.

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