Review Board 1.0.8

Part 1 of merge r1025:r1064 from release branch into trunk: Some modifications to Repository library

Updated 8 months, 2 weeks ago

Severin Gehwolf Reviewers
trunk markus_developers
None MarkUs Source Code Repository 
When I merged the changes from the release_0.5 branch into trunk and ran the unit tests, I got approximately 90 failures/errors. The reason for all these changes are: Get test to pass again. Simplify/Unify repository library configuration. It takes a configuration hash now, when the factory is called (e.g. Repository.get_class("svn", conf)). I also removed the repository_factory.rb file and merged it into repository.rb

Please take your time when looking at it (there are more parts to come) Aside: The diff of my local changes 3000 something lines is probably to big for ReviewBoard. It seg-faults or whatever when trying to create a review with the large diff. Feedback is very welcome!
for this part:

lib/repo/test/memory_repository_test.rb
lib/repo/test/subversion_repository_test.rb

both pass.
Severin Gehwolf
Posted 8 months, 3 weeks ago (November 8th, 2009, 4:29 p.m.)

   

  

   
    
    
   


  




   



    

     
     
    

    

     
Since it is now called with absolute paths to repositories. That's why I use File.basename here now. Mike, hope this is OK. It makes things a lot easier for testing using MemoryRepository.

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Added the "__" prefix to the class methods, since they shouldn't be called directly...

     

  

   
    
    
   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Mike Conley

  
Posted 8 months, 3 weeks ago (November 8th, 2009, 11:03 p.m.)

 

 

   
I've got really minor critiques below.  It's absolutely brilliant - this is huge work, Severin.  Great job!

   

  

   
    
    
   


  




   



    

     
     
    

    

     
Yes, this looks wayyy better. :)

     

  

   
    
    
   


  



    

    

    

     
     

      

       
        

         
          trunk/lib/repo/repository.rb
          

           (Diff revision 1)

          
         
        

       
       

        
 


       
      

     

    

    

     
I'd suggest putting those strings into an Array, like this:

CONFIG_KEYS = ['REPOSITORY_PERMISSION_FILE', 'IS_REPOSITORY_ADMIN', 'REPOSITORY_STORAGE']

if CONFIG_KEYS.include?(k)
  @CONF[k.to_sym] = v



     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Who said stuff?  It was probably me.  :)

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Yeah, I like Repository.conf way better.  Great idea.

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
I'd put spaces on either side of that +  for readability.

     

  

   
    
    
   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Severin Gehwolf

  
Posted 8 months, 3 weeks ago (November 9th, 2009, 7:51 a.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     

      

       
        

         
          trunk/lib/repo/repository.rb
          

           (Diff revision 1)

          
         
        

       
       

        
 


       
      

     

    

    

     
Nice idea, thanks!

     

  

   
    
    
   


  



    

    

   


  
 




  

 














 

  




 

   Review request changed
   
Updated 8 months, 3 weeks ago (November 9th, 2009, 11:09 p.m.)

 

 

  

  
  
Changed according to feedback, added some more tests. Fixed defined? problem. It doesn't work for a Hash (if the key does not exist, nil is returned).


 




  

 















 

  




 

 

  
Ship it!

  
Mike Conley

  
Posted 8 months, 2 weeks ago (November 11th, 2009, 11:57 a.m.)

 

 

   
I'm happy with this.  It's a lot of code to look at, but thankfully, a lot of it was just shuffled around.  That's 2/3...one more to go!