Opened 6 years ago
Closed 6 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 , 6 years ago
follow-up: 3 comment:2 by , 6 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.
comment:3 by , 6 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 , 6 years ago
Patch is finished. Pull request is submitted. https://github.com/django/django/pull/10836
comment:5 by , 6 years ago
Has patch: | set |
---|
comment:6 by , 6 years ago
Patch needs improvement: | set |
---|
Documentation fails its build, because django is considered as misspelling.
comment:7 by , 6 years ago
Patch needs improvement: | unset |
---|
Fixed documentation. All checks have passed.
comment:8 by , 6 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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.
Now I am working on tests and documentation.