Remove sys.exit from django-evolution evolve

 






 

 


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

 


















 

  

   Information

  





 
 

  mike_conley
 









 
 

  Django Evolution (deprecated)
 









 
 

  trunk
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  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




   

   





  

 



 

  


   

   

    
 
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. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




mike_conley




   

   





  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




edufelipe




   

   





  

 



 

  


   

   

    
 
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. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




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
 
    /trunk/django_evolution/management/commands/evolve.py 
 

   		
  



 










  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...