RB Global Changes

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

Information

RBTools
master

Reviewers

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
  1. 
      
  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.
  3. 
      
SL
  1. 
      
  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. Ah, I see; I think a more Pythonic way might be:
      
              repository_url_exists = self.options.repository_url is not None
      
      for readability
    3. Haha! Nice. A lot better, thanks!
  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. I think it's decomposed into System > 3rd Party > RB groupings, and then lexicographical sort.
      
      So what that looks right, yeah.
  4. 
      
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. 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. 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. 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. 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.
  2. 
      
JS
JS
JS
JS
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. 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. rbtools/commands/__init__.py (Diff revision 5)
     
     
     
    Show all issues
    Alphabetical.
  4. rbtools/commands/__init__.py (Diff revision 5)
     
     
    Show all issues
    Trim the spaces.
  5. 
      
SM
  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. 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.
  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. 
      
JS
mike_conley
  1. Looks good to me. Thanks John!
  2. 
      
mike_conley
  1. Ship It!
  2. 
      
david
  1. Just one little comment.
  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.
  3. 
      
JS
JS
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. 
      
JS
SM
  1. Ship It!
  2. 
      
JS
Review request changed
Status:
Completed
Change Summary:
Pushed to api (cbe485c173).