Remove sys.exit from django-evolution evolve

Review Request #1633 — Created June 3, 2010 and submitted

mike_conley
Django Evolution (deprecated)
trunk
django-evolution, reviewboard
When django-evolution evolve hits a problem, it currently dumps out the error string, and then uses sys.exit to escape.  This is problematic when attempting to leverage django-evolution programmatically, since we really don't want to have to catch the SystemExit exception.

The preferred way of signaling when something has gone wrong with a Django command is with the CommandError exception.  I've replaced all instances of sys.exit with this exception.

I've also taken the code out of handler, and put it in a new method called evolve.  This will make it much easier to call evolve programmatically, and to catch exceptions.
All tests pass.
mike_conley
  1. Hm - may have been a bit rash, here.  It looks like when Django runs a BaseCommand, it catches any CommandError's and sys.exit's anyways. (See django.core.management.BaseCommand's execute method)
    
    Looks like a blind alley.  I'm discarding this review request.
    1. Got a nice solution from ChipX86 - extracting evolve code into a new method called "evolve", and having handle call it.
  2. 
      
mike_conley
  1. 
      
  2. /trunk/django_evolution/management/commands/evolve.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Hm.  Any suggestions on how to get all of those strings into the CommandError?
    
    I had originally started by creating an empty string variable called error_string, and then replacing all of those prints with:
    
    error_string += ...
    
    Didn't like the looks of it though.  Open to suggestions.
    1. The new command shouldn't contain all th code that the old evolve management command contains. Rather, it should be an API that returns useful data, in the form of returned results or, on failure, exceptions (probably using a new EvolutionFailed class). The management command would call this and then display that text with the provided results from the exception or returned data, and our web UI could display it's own formatted data however we choose and with our own formatting.
  3. 
      
edufelipe
  1. Personally I think that catching a SystemError is better. Just because other things like django.core and distutils are code we cannot change, and they might exit.
    That's just my two cents.
    1. Hey Eduardo:
      
      Worst-case scenario, it might come to that.  ChipX86 and purple_cow have suggested this approach though - we'll see how it goes.
      
      Thanks for the review,
      
      -Mike
  2. 
      
mike_conley
Review request changed

Change Summary:

There was one CommandError that wasn't returning a message - I've updated it to return something a little more useful.

Diff:

Revision 3 (+11 -10)

Show changes

chipx86
  1. Looks good, thanks! Committed to Django Evolution (r183).
  2. 
      
Loading...