Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assets management command swallows errors #51

Open
qris opened this issue Dec 16, 2014 · 0 comments
Open

Assets management command swallows errors #51

qris opened this issue Dec 16, 2014 · 0 comments

Comments

@qris
Copy link
Contributor

qris commented Dec 16, 2014

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant