Enhancement to the call_command function to allow the use of modules as arguments

[Mike Lissner]

I recently filed a bug about this and was redirected here for discussion. While using call_command (which simplifies calling management commands), it occurred to me that the API is a little strange. It currently is designed to work like this:

    call_command('my_command', *args, **kwargs)

The problem here is that you pass a string into your command, and then Django does the magic of converting that into something that can be imported, and then uses argparse to parse args and kwargs. I think a better API would be:

    from my_project.my_app.management.commands import my_command
    call_command(my_command, *args, **kwargs)

There are three big advantages of this. First, if you ever change the name of your command, a good IDE can realize that it needs to update the import statements, and that'll happen automatically. This is good and important in larger projects where you can't keep all the code in your head.

Second, this allows code completion from your IDE, making it less likely to make a typo or a mistake. Another good thing.

Third, this reduces the amount of string to module importing that Django does, and less magic is generally good.

In terms of process, I propose we follow the standard deprecation process. At first, it should accept either input, and issue a warning. Over time, it should only allow modules.

The bug I filed was closed saying that, there wasn't "sufficient justification for the additional complexity that would be required." But this should only take a few lines of code to check what the argument is (string or module), and then to do the conversion.

I'm curious what the group thinks. This seems like a clear improvement to the API to me, but perhaps there's something I'm missing. Happy to discuss.

[Aymeric Augustin]

Hello Mike,

I saw your ticket a few days ago, thanks for bringing it up here.

> While using call_command (which simplifies calling management commands), it occurred to me that the API is a little strange. It currently is designed to work like this:
>
> call_command('my_command', *args, **kwargs)

The API is designed like this because it performs the equivalent of `django-admin my_command [arg] [arg] […] [--kwarg] [--kwarg] […]`.

> The problem here is that you pass a string into your command, and then Django does the magic of converting that into something that can be imported, and then uses argparse to parse args and kwargs. I think a better API would be:
>
> from my_project.my_app.management.commands import my_command
> call_command(my_command, *args, **kwargs)

In general I would suggest to factor your business logic into a function that you can call independently from the management command. This is better for testing as well.

Then the management command is just a thin wrapper that validates the command line parameters and calls that function with corresponding arguments.

> There are three big advantages of this. First, if you ever change the name of your command, a good IDE can realize that it needs to update the import statements, and that'll happen automatically. This is good and important in larger projects where you can't keep all the code in your head.

Management commands are usually called from outside the Python project. If you rename a management command, “Find -> Replace All” in your source code is probably going to take less time than updating all external scripts or procedures that depend on it.

> Second, this allows code completion from your IDE, making it less likely to make a typo or a mistake. Another good thing.

Fair enough. [I don’t use an IDE myself. I write tests ;-)]

> Third, this reduces the amount of string to module importing that Django does, and less magic is generally good.

We won’t gain much because django-admin my_command will still need to find my_command in one of the installed apps.

> In terms of process, I propose we follow the standard deprecation process. At first, it should accept either input, and issue a warning. Over time, it should only allow modules.

I’m -1 on removing the Python API that’s equivalent to `django-admin my_command`. It’s needed for testing management commands that override other management commands.

An alternative would be to split call_command() in two functions: one that locates the command and one that figures out the default values of the arguments. The latter is the piece of code you want to reuse.

> The bug I filed was closed saying that, there wasn't "sufficient justification for the additional complexity that would be required." But this should only take a few lines of code to check what the argument is (string or module), and then to do the conversion.

Django used to have lots of APIs that accepted multiple types of arguments. We tend to remove them when possible because they’re a source of bugs and confusions and also because there should be one way to do each task. I’m -0 on taking the opposite route here. This is a theoretical argument that I’ll happily withdraw if a quick type check is the practical option :-)

--
Aymeric.

[Mike Lissner]

Thanks for the response, Aymeric. Comments inline.

Mike

On Sunday, August 16, 2015 at 1:41:19 PM UTC-4, Aymeric Augustin wrote:

In general I would suggest to factor your business logic into a function that you can call independently from the management command. This is better for testing as well.

This is the second time I've seen this recommendation recently, but I have to say it wasn't how I thought about writing management commands (would make testing easier though, I agree). There are docs about testing management commands, and they are very brief, only recommending that you use call_command.

Management commands are usually called from outside the Python project. If you rename a management command, “Find -> Replace All” in your source code is probably going to take less time than updating all external scripts or procedures that depend on it.

True. But if it's just a string in your code, some forms of find/replace won't catch it. In Intellij, for example, if you rename a file, it'll automatically fix imports for you. That'll give you the confidence that things worked, but you'll miss any places where it was used as a string. Full find/replace would catch that, but depending on the name of your command, you may not be able to easily run a full find/replace (e.g., your command had the same name as a field in your model).

In general, I try not to pass strings when I mean modules or classes, and call_command violates that.

 

> Second, this allows code completion from your IDE, making it less likely to make a typo or a mistake. Another good thing.

Fair enough. [I don’t use an IDE myself. I write tests ;-)]

Me too, though sometimes there aren't enough tests (there are never enough tests).

 

> Third, this reduces the amount of string to module importing that Django does, and less magic is generally good.

We won’t gain much because django-admin my_command will still need to find my_command in one of the installed apps.

Sure. Though that magic is necessary and this magic is avoidable.

 

I’m -1 on removing the Python API that’s equivalent to `django-admin my_command`. It’s needed for testing management commands that override other management commands.

Not sure what you mean here, but I suspect you're making a good point. Does seem pretty niche though. Can you explain?

 

An alternative would be to split call_command() in two functions: one that locates the command and one that figures out the default values of the arguments. The latter is the piece of code you want to reuse.

I'd be +1 for that, though losing the convenience of call_command would be unfortunate.

After reading this, my feeling is that allowing call_command to accept (and recommend) modules is still a better way to go in most cases. Passing a string just feels dirty and sloppy if it's unnecessary. Simply adding support for modules as arguments seems simple and it'd knock out one small source of bugs.

If anybody else agrees, I'd be happy to get this coded up. I'll move on though, should another person give me the -1. I've long learned that the Internet is generally wiser than I am!

Mike

[Aymeric Augustin]

2015-08-17 16:54 GMT+02:00 Mike Lissner <mlis...@michaeljaylissner.com>:

I’m -1 on removing the Python API that’s equivalent to `django-admin my_command`. It’s needed for testing management commands that override other management commands.

Not sure what you mean here, but I suspect you're making a good point. Does seem pretty niche though. Can you explain?

Here's the scenario.

1. You write a do_stuff management command in your project. You write a test to ensure that call_command('do_stuff') does the right thing.

2. A co-worker adds a third-party app which also implements a do_stuff management command. Unfortunately, the order of INSTALLED_APPS means that your command is overridden by the third-party app. Fortunately, your test catches the problem.

With the change you're proposing, the test wouldn't catch the problem anymore.

That's why Django needs an API to emulate `django-admin do_stuff`.

--

Aymeric.

[Mike Lissner]

I see. Could this concern be addressed by adding it to the checks framework, so that it throws a warning if there are ever two commands with the same name?

--
You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/8aNf-lZXSVg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CANE-7mVHu8cCw8dUDcA4ECnNqCGD%2BS4LAPBFRnBUAo3%2Bm65PcA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Marc Tamlyn]

This is a deliberate approach you would use - South used it for years to customise syncdb.

call_command is intended as a python API for testing `$ django-admin foo`. Two of your arguments are based around IDE usage, which I don't think is a valid argument, and the third is about reducing string magic. I don't feel there is any magic at all here, we are modelling a string based API so naturally we use strings to do so.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMp9%3DEzBJSSyhc7F_recj4-8CUxghzXSG%3Drx9EBT6MtDm3AeaQ%40mail.gmail.com.

[Carl Meyer]

Hi Marc, Mike, Aymeric,

On 08/18/2015 06:52 AM, Marc Tamlyn wrote:
> This is a deliberate approach you would use - South used it for years to
> customise syncdb.
>
> call_command is intended as a python API for testing `$ django-admin
> foo`. Two of your arguments are based around IDE usage, which I don't
> think is a valid argument, and the third is about reducing string magic.
> I don't feel there is any magic at all here, we are modelling a string
> based API so naturally we use strings to do so.

I agree that the way Django maps string names to management commands is
reasonable and useful and not especially magical -- if you're building a
CLI you have to map strings to code in some way.

I also fully agree with Mike that usually in Python code it's preferable
to explicitly import and call the code you want, rather than going
through an extra string-to-code mapping (which only exists to support a
CLI). For this reason, I rarely use `call_command`. But I don't think it
should be changed. There needs to be some API (if only for testing,
which is the only place I ever use it) that exercises a management
command end-to-end, including the name-to-code mapping, the default CLI
arguments and options, etc. `call_command` is that API, and it does that
(necessary) job well, so there's no reason to change it.

The responsibilities of a management command class should be to parse
input (CLI args and options) and manage output (stdout/stderr).
Everything else (all business logic) should be extracted into
independently reusable functions or classes. If you need to exercise the
whole CLI-oriented shebang (including string-to-code mapping,
args/options parsing, and stdout/stderr), you use `call_command`. If you
just want the business logic, don't mess with the management commands
system at all; just import and use your regular Python functions and
classes.

(Doc patches to better reflect that principle in the management command
docs are welcome, IMO.)

Carl

[Mike Lissner]

On Tuesday, August 18, 2015 at 12:00:30 PM UTC-4, Carl Meyer wrote:

(Doc patches to better reflect that principle in the management command
docs are welcome, IMO.)

Yeah, it's sounding like this is the change that's needed here. Probably the place to do that is here:

https://docs.djangoproject.com/en/1.8/topics/testing/tools/#management-commands

Perhaps should just point out that:

1. call_command should be used if you want to do end-to-end parsing.
2. business logic should be placed outside of the Command class in easy to grab functions.
3. only parsing logic should live in the command itself.

It'd also be good to update the management commands documentation to explain how business logic should live outside the command itself. That'd not how they're presently described. The current example has:

    def handle(self, *args, **options):
        for poll_id in options['poll_id']:
            try:
                poll = Poll.objects.get(pk=poll_id)
            except Poll.DoesNotExist:
                raise CommandError('Poll "%s" does not exist' % poll_id)

            poll.opened = False
            poll.save()

            self.stdout.write('Successfully closed poll "%s"' % poll_id)

 

[Carl Meyer]

On 08/18/2015 10:10 AM, Mike Lissner wrote:
> On Tuesday, August 18, 2015 at 12:00:30 PM UTC-4, Carl Meyer wrote:
>
>
> (Doc patches to better reflect that principle in the management command
> docs are welcome, IMO.)
>
> Yeah, it's sounding like this is the change that's needed here. Probably
> the place to do that is here:
>
> https://docs.djangoproject.com/en/1.8/topics/testing/tools/#management-commands
>
> Perhaps should just point out that:
>
> 1. call_command should be used if you want to do end-to-end parsing.
> 2. business logic should be placed outside of the Command class in easy
> to grab functions.
> 3. only parsing logic should live in the command itself.

Yep.

> It'd also be good to update the management commands documentation to
> explain how business logic should live outside the command itself.
> That'd not how they're presently described. The current example has:
>
> def handle(self, *args, **options):
> for poll_id in options['poll_id']:
> try:
> poll = Poll.objects.get(pk=poll_id)
> except Poll.DoesNotExist:
> raise CommandError('Poll "%s" does not exist' % poll_id)
>
> poll.opened = False
> poll.save()
>
> self.stdout.write('Successfully closed poll "%s"' % poll_id)

IMO the first four lines there are arguably part of parsing CLI input --
they are validating the given poll ID and converting it a Poll model
instance. I would usually leave that code in the management command,
since my business logic API likely prefers to operate in terms of model
instances, not their IDs.

So the "business logic" in this toy example really boils down to just
one or two lines: marking a poll opened=False and then saving it. Hardly
worth extracting; that's the problem with toy examples. Perhaps just a
prose note could be added mentioning that as the business logic expands,
it should be extracted.

Carl