post-review: Args required in reviewboard should be enforced in the command line

 






 

 


  Review Request #1410
  —
  Created Feb. 11, 2010 and discarded

 


















 

  

   Information

  





 
 

  psa
 









 
 

  RBTools
 









 
 

  
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Require summary, description and a set of reviewers

 








  


 
 

  

 







 









 





  








 









 






  

 








 














 



 

  

   





 PS





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  















  

   

    Summary:
   

   

    
Args required in reviewboard should be enforced in the command line
post-review: Args required in reviewboard should be enforced in the command line

   

  












  

 
















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    There shouldn't be any requirement for specifying/guessing a summary, description, testing done or groups on the command line. These are optional and in a lot of cases it's not even what you'd want.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Part of the reason for this patch was to bring post-review in line with the web interface.

The web interface (at least a default install of 1.1alpha2) forbids publishing reviews if you haven't met these requirements but post-review does not and the inconsistency grated on me (and I can't think of a good reason you should be posting a review without at least a summary and someone to do the review).
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      You can post one on the web UI without that information. You just can't publish it. That's the same with post-review.

The publishing validation must be server-side regardless. It's perfectly okay (and common) to post a review request without this information and then to edit it on the server. I do this with nearly every change, for instance. And with some repository types (such as Perforce) the server actually supplies this information, meaning you wouldn't want to either specify this or guess in post-review.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      It is possible to run "post-review -p" and publish without meeting any of the requirements. It was this realization that lead to the patch.

I do agree though that it should also be server side. I'm still inclined towards doing checks at the client level too.

For things like Perforce, maybe a check that says not to enforce this for servers where reviewboard will collect that information from the repository (or, I've not played with it, does reviewboard collect it or does it need to be forced to do so?)?
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      How about we just require them with -p?
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      Even then, the server should enforce them. Just because you passed -p and didn't pass --summary or something doesn't mean that the server doesn't have a summary and other necessary information.

I think if we pass --publish, and we get an error from the server (which we must make sure we enforce, and I think we do in 1.5?), then we should yell. Otherwise, I don't think there's any value of checking these options prior to calling the server.
      
 
      

      6. 



    6. 
 
 
      
 
      
  
 
      
 
      
  
      I just checked against 1.5B1 and it doesn't enforce it yet.

post-review --server=http://reviews2. -p
Review request #3 posted.

http://reviews2/r/3

      
 
      

      7. 



    7. 
 
 
      
 
      
  
 
      
 
      
  
      Yeah.. We definitely need to fix that server-side then.
      
 
      

      8. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3.