post-review for P4win




 


  Review Request #467
  —
  Created July 24, 2008 and submitted

 















 
  
  
 




 
  

   Information

  

 
 

  

   
   
JSlominski


  


  

   
   
Review Board SVN (deprecated)


  


  

   
   



  


  

   
   



  


  

   
   



  


 



 
  

   Reviewers

  

 
 

  

   
   
reviewboard


  


  

   
   



  


 











 

  
  


I have updated post-review to set the proper perforce environment.  Before P4Tool.txt is imported into P4win, the user needs to enter the path of post-review.  After it is imported they should be able to right click on any change list and by clicking post-review, post a review of the change list in perforce.






 




 

  
  






 














 

 














  

 








 














 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Given that this is a tool that needs to be compiled, it'd be nice to make this its own top-level module in /trunk and add a Makefile for compiling it.

Also, is there any reason we need a wrapper? The script can't just run python post-review or maybe a compiled post-review.exe?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      The reason I out this in a wrapper is because post-review will take the client and server that is set in the P4 ENV variables.  By setting up the environment properly before running python post-review, users can submit reviews from multiple client specs without any hassle.  I’m not sure how other places use clients but we have a separate client for each branch of software we release from.  I could set up a compiled version of this but then I would need to make the command line arguments more robust.  If you think that would be a better approach I can start working on that on Monday. 
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Maybe what we should do is have some Perforce-specific arguments in post-review for setting these variables instead of getting them from the environment.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Should I abandon this and make post-review.py more robust?
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry, missed your question before.

Yeah, it might be worth making post-review more robust so that it can be used in other ways. Having these Perforce-specific arguments could be useful in all kinds of scripts.
      
 
      

      5. 


 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
    You can use setenv for this.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I know setenv() works on Unix platforms but windows didn't like it.  I took the easy route and did this.  Ill do some more digging on Monday to see if I can find the proper function. 
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Fixed this for the windows platform
      
 
      

      3. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
    This line can go.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Not really sure how this got here
      
 
      

      2. 


 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
    You don't need "!= None" here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
    You should be using isinstance() here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
    I'd prefer --p4-client and --p4-port. The variable names should be p4_client and p4_port too.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
    I'd rather we not abbreviate "CL". This is more generic than changelists, so let's just say something like "the Perforce client name" and "the Perforce server address" or something.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks, committed as r1550.

I don't use Windows and therefore don't use P4win, so I can't test this, but what happens if you need to log in? Does P4win provide any sort of a terminal UI where you can input this information, and does it show the resulting URL?

We may want to modify this to use -o (for opening a web browser) after this is posted.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...