miracle2k / django-assets

Django webassets integration.
BSD 2-Clause "Simplified" License
89 stars 79 forks source link

Assets management command swallows errors #51

Open qris opened 9 years ago

qris commented 9 years ago

If you run manage.py assets with a command or options that aren't recognised, it prints an error but doesn't set a return code properly:

$ ./manage.py assets --invalid
usage: manage.py assets [-h] {watch,build,clean,check} ...
manage.py assets: error: too few arguments

$ echo $?
0

This makes it hard for scripts (like our deployment scripts) to detect that it failed with an error instead of succeeding.

I can see that django_assets.management.commands.assets.Command.handle() expects commands to throw some kind of exception if they fail (that seems to be the only way to get a non-zero return code out of handle()):

def handle(self, *args, **options):
    ...
    try:
        impl.run_with_argv(args)
    except AssetCommandError as e:
        raise CommandError(e)

and that webassets.script.Command.run_with_argv() swallows SystemExit and returns 1 instead:

def run_with_argv(self, argv):
    try:
        ns = self.parser.parse_args(argv)
    except SystemExit:
        # We do not want the main() function to exit the program.
        # See run() instead.
        return 1

but I don't know which behaviour is correct. Should we catch a nonzero return code in handle and throw an AssetCommandError to propagate the exception out? Is it really wise to swallow and regenerate such exceptions?

IMHO, it's better not to throw SystemExit unless you mean it (thus, never in code that can be called by a library) and better not to hide and rethrow exceptions. Perhaps the django_assets Command should be using a lower-level interface to webassets rather than calling its run_with_argv method?