RB Global Changes

 






 

 


  Review Request #3612
  —
  Created Dec. 2, 2012 and submitted 

 


















 

  

   Information

  





 
 

  jsintal
 









 
 

  RBTools
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  rbtools
 









 
 

  
 






 








 





  


 
 

  
Addresses some "pull up refactoring" of rb command based code and illegal/invalid attribute access (rectified with getattr(options, "field", default_value) method).

Please apply this patch before applying any of the newer RB commands (patch [r/3440], attach[r/3599], close[r/3595], publish, diff[r/3429]).



 








  


 
 

  
Individual testing of each command that branched off of this feature was tested for functionality.
Heavily tested in both SVN, Git, and some Hg.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
print the error seems harmless, but should I log it instead? This is the equivalent to user not providing any …

      		
      
       
 JS
jsintal
      
      
       
      
     




     

      
       
       
Alphabetical.

      		
      
       
 SL
slchen
      
      
       
      
     




     

      
       
       
While this works, the flag is a bool, so the fallback should be False.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Alphabetical.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Trim the spaces.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
I don't know how I feel about this happening inside get_server_url. It's a side effect which isn't obvious. How about …

      		
      
       
 SM
smacleod
      
      
       
      
     




     

      
       
       
This needs to be a little more complex. Take a look at how "rb open" is instantiating the client. It …

      		
      
       
 SM
smacleod
      
      
       
      
     




     

      
       
       
Alignment here is a tad off, because of the nested parens. It should be: if ((getattr(options, 'p4_client', None) or getattr(options, …

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 








 














 



 

  

   





 JS





   

   

  

 



 















 





 

  

   





 JS





   

   







  

 



 

  


   

   

    
 
jsintal


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/clients/git.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    print the error seems harmless, but should I log it instead? This is the equivalent to user not providing any guess_description even if it did exist.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Now, I am only printing it if --debug is enabled. Much better.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   





 SL





   

   







  

 



 

  


   

   

    
 
slchen


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/clients/git.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    What does the 'not not' do here?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It may look a little crazy. Basically, it's like: !(var == None)
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Ah, I see; I think a more Pythonic way might be:

        repository_url_exists = self.options.repository_url is not None

for readability
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Haha! Nice. A lot better, thanks!
      
 
      

      4. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/commands/__init__.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Alphabetical.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Hmm, I think I messed up scan_usable_client vs RBClient or do we organize imports by the file path (and not the imported class, method)?

Should it be:
from rbtools.*api.client import RBClient	
from rbtools.*api.errors import APIError, ServerInterfaceError
from rbtools.*clients import scan_usable_client	
from rbtools.*utils.filesystem import get_home_path, load_config		
from rbtools.*utils.process import die	

      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I think it's decomposed into System > 3rd Party > RB groupings, and then lexicographical sort.

So what that looks right, yeah.
      
 
      

      3. 


 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    It looks like you're adding all the AttributeError checks for when the options structure doesn't contain certain items, but I would much rather try to eliminate the calls to these methods that use them entirely when they're not needed.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Right now, these "if statements" assume that a certain option field exists, while in most cases for commands it doesn't exist. So, if the option field does not exist, then we can pretend that in the case it did exist, it was of a None value. These two states are equivalent (being None or not existing as an option field). However, I agree David, it's not the best solution at the moment. I am trying to find a better way. If you have any pointers on how this may be accomplished, please let me know. 
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Doing some Googling on 'AttributeError,' perhaps I could do the change globally in the 'options' .__getattr__(self, attr) and have that return None instead of raising an error. http://stackoverflow.com/questions/2352630/what-special-method-in-python-handles-attributeerror . What do you guys think?
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      I think that's still just papering over the problem. Much better would be to move the checks that are causing you trouble up the stack to the commands that need them. For example, the --change-only flag is probably only useful for "rb post", so the check could be moved into that command after scan_usable_client is called.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      Alright. I had just spent a lot of time/effort on changing all the options field grabbing into the getattr() function. I will see if I can find a better way, as you suggested, tomorrow if time persists. At least the current solution works.
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      Okay. The getattr approach is already significantly better so don't feel too pressured if you have other things you want to get to before the pencils-down time.
      
 
      

      6. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 JS





   

   

  

 



 















 



 

  

   





 JS





   

   

  

 



 















 



 

  

   





 JS





   

   

  

 



 















 



 

  

   





 JS





   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This is looking pretty good. A few very tiny things and then I'm happy.

We'll want to down the road split up some of those diff functions, I think, so that we're not trying to generate the guessed fields while doing the diff, but that's not important for any of your changes.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/clients/__init__.py
       


        (Diff revision 5)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    While this works, the flag is a bool, so the fallback should be False.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/commands/__init__.py
       


        (Diff revision 5)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Alphabetical.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/commands/__init__.py
       


        (Diff revision 5)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Trim the spaces.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 





 

  

   





 SM





   

   







  

 



 

  


   

   

    
 
smacleod


    
   


  

  

  



    




  1. 
 
    
  
    
   
    In the future we'll definitely want to put some solid effort into dealing with the option dependencies, but for now I'm happy with the calls to getattr.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/commands/__init__.py
       


        (Diff revision 5)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I don't know how I feel about this happening inside get_server_url. It's a side effect which isn't obvious.

How about we make this method take the tool and respository_info as arguemnts.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Totally agree, however it seems that initialize_scm_tool() and get_server_url() are tightly coupled (get_server_url directly depends on 'tool'). I am rather new to Python, but I know the self.fields aren't guaranteed to be created at the time of running get_server_url(), which is why I put it there.

But I will follow your advice and pass as args from the actual cmd.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/commands/__init__.py
       


        (Diff revision 5)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This needs to be a little more complex. Take a look at how "rb open" is instantiating the client. It provides an auth callback which is used to ask for a username and password if something needs authorization. There should at least be a way to pass it into get_root.

We should also probably refactor the auth callback I used for "rb open" into a common place, and use it as a default here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   





 JS





   

   

  

 



 















 





 

  

   




mike_conley




   

   





  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good to me. Thanks John!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




mike_conley




   

   







  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Just one little comment.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       rbtools/clients/__init__.py
       


        (Diff revision 6)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Alignment here is a tad off, because of the nested parens. It should be:

if ((getattr(options, 'p4_client', None) or
     getattr(options, 'p4_port', None)) and
    not isinstance(tool, PerforceClient)):
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Maybe also have the isinstance check first.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   





 JS





   

   

  

 



 















 



 

  

   





 JS





   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good to me. I'm going to let smacleod push your changes because it looks like the rb-commands isn't in the upstream repo yet.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 JS





   

   

  

 



 















 





 

  

   





 SM





   

   







  

 



 

  


   

   

    
 
smacleod


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 JS





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  







  

   

    Status:
   

   


    Completed

   

  




  

   

    Change Summary:
   

   

    
Pushed to api (cbe485c173).