Pull out code of SCM clients from postreview into separate files

Review Request #2399 — Created June 14, 2011 and submitted

Information

RBTools

Reviewers

As the preliminary step for releasing 'api' branch SCM clients code from postreviews is pulled out into rbtools/clients/*. In the future those clients should be merged with ones from 'api', for now it lets us separate theirs code and hence simplify maintaining of 'api' clients up-to-date with master's.

git history is at http://github.com/mbait/rbtools/tree/new-clients
rbtools/tests.py passes all tests except two suspicious which fail on master and perhaps should be reviewed
Description From Last Updated

Two blank lines.

chipx86chipx86

Can you change this to logging.error?

chipx86chipx86

Not your fault, but while you're here, "Repository's"

chipx86chipx86

Can you fix this indentation?

chipx86chipx86

I think the contents of this file should also go in clients/__init__.py. That way, clients is the base module for …

chipx86chipx86

logging.error

chipx86chipx86

logging.info

chipx86chipx86

The first rbtools.clients should be grouped with the others, in alphabetical order.

chipx86chipx86

This should go before the others. Shallow depth first.

chipx86chipx86

Import only what you need.

chipx86chipx86

Actually, importing a global is kinda bad. Instead, I'd add a new clean_tempfiles function in rbtools.utils.files, which die() can call.

chipx86chipx86

Should be two blank lines before this, rather than one.

chipx86chipx86

So this will actually set the local variable and not the global one. Did you test post-review after this change? …

chipx86chipx86

Same here. All other files too.

chipx86chipx86

RepositoryInfo moved, so this shouldn't still work. Make sure you've deleted the client.pyc file and re-run both pyflakes and the …

chipx86chipx86

Two blank lines.

chipx86chipx86
chipx86
  1. Thanks for working on this. There's a number of things I'd like cleaned up though. I think much of this came from that branch, but one thing that's important to know going forward is that that code wasn't fully reviewed and updated, so there's going to be lots of restructuring happening. Partly why it'll be great to have this broken up into different things.
    
    So a few file-based things not otherwise covered here.
    
    1) rbtools/api/utilities.py is probably not the right place for this. These are really utilities used by the client. I'd actually go further than a single file. Create rbtools/utils/*.py files. A good start would be checks.py (check_install, check_gnu_diff), files.py (make_tempfile, walk_parents), process.py (execute). I think a lot of the utils we have in there aren't actually needed, so we can nuke those.
    
    2) The contents of getclient.py should move to clients/__init__.py. That should be the highest import point for anything using the clients API.
    
    3) This will need thorough testing. I've already noticed a few places where things like "execute" are called without "self.util.execute". That's okay, since I want self.util and RBUtilities to go away, but something to note.
    
    You can sanity check a lot of that by running `pyflakes rbtools` from the top source directory. You may need to install pyflakes first.
    
    Having new unit tests would also be great, if possible. But I'd do the rest of this and get a new review out first.
  2. rbtools/api/utilities.py (Diff revision 1)
     
     
     
     
    No blank line.
    
    Might as well call tempfile.mkstemp instead.
  3. rbtools/api/utilities.py (Diff revision 1)
     
     
    No point in having a class here. The module suffices. Every function in here should just be a top-level function, and all import sites should import the specific functions needed.
  4. rbtools/api/utilities.py (Diff revision 1)
     
     
    Is this used? How about the other functions? Let's only put in here what we need for this specific change. I suspect most of these we won't want to keep anyway.
  5. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    Should be a from/import.
  6. rbtools/clients/clearcase.py (Diff revision 1)
     
     
    While you're here, please change this to:
    
    super(ClearCaseClient, self).__init__()
  7. rbtools/clients/client.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  8. rbtools/clients/client.py (Diff revision 1)
     
     
    I don't think we should do the util thing. At some point we'll want to clean up the SCMClients so that some of the functions they otherwise call from util will go through another mechanism, but I don't want util there in the meantime.
  9. rbtools/clients/client.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  10. rbtools/clients/client.py (Diff revision 1)
     
     
    Inherit from object.
  11. rbtools/clients/client.py (Diff revision 1)
     
     
     
     
    This should be able to just use debug(), which can be in the utils file. Same with all others.
  12. rbtools/clients/client.py (Diff revision 1)
     
     
    This should be in postreview.
    1. walk_parents is called from svn.py also so circular dependencies at this point.
  13. rbtools/clients/client.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  14. rbtools/clients/client.py (Diff revision 1)
     
     
    This should be in postreview.
  15. rbtools/clients/client.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  16. rbtools/clients/client.py (Diff revision 1)
     
     
    This should be in postreview.
  17. rbtools/clients/cvs.py (Diff revision 1)
     
     
    Should use super.
  18. rbtools/clients/cvs.py (Diff revision 1)
     
     
     
    Indentation is wrong, but will be fixed when calling execute directly and not self.util.execute.
  19. rbtools/clients/getclient.py (Diff revision 1)
     
     
     
     
     
     
     
     
    These shouldn't have this alignment.
  20. rbtools/clients/getclient.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  21. rbtools/clients/getclient.py (Diff revision 1)
     
     
     
    This is maybe not the right name for this, since what we're doing is scanning for the appropriate client giving a checkout. Instead, maybe call this "scan_usable_client". I feel there should be a better name, but I can't think of it right now.
  22. rbtools/clients/getclient.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    These should probably not be here. This function is intended to get the client, in a working state, not validate options. That should be elsewhere. In fact, ideally, SCMClient would have a validate_options function that does this, and we can just pass the options (in postreview) to this.
  23. rbtools/clients/git.py (Diff revision 1)
     
     
    super
  24. rbtools/clients/mercurial.py (Diff revision 1)
     
     
    super
  25. rbtools/clients/perforce.py (Diff revision 1)
     
     
    super
  26. rbtools/clients/plastic.py (Diff revision 1)
     
     
    super
  27. rbtools/clients/plastic.py (Diff revision 1)
     
     
     
     
    One blank line.
  28. rbtools/clients/svn.py (Diff revision 1)
     
     
    super
  29. rbtools/postreview.py (Diff revision 1)
     
     
     
     
    Imports should always be in alphabetical order.
  30. rbtools/postreview.py (Diff revision 1)
     
     
    This leads to too much to type. It should always be a "from * import *". I'll have some suggestions below about what we're importing.
  31. rbtools/postreview.py (Diff revision 1)
     
     
     
    < 80 chars.
    
    For multi-line if statements, wrap in the form of:
    
    if (foo and
        bar and
        baz):
    
    Also, I know it was this way before, but can you make this: If (DEBUG or (options and options.debug): ?
  32. rbtools/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Alphabetical order, and no blank lines between any of these.
    
  33. rbtools/tests.py (Diff revision 1)
     
     
     
     
    No blank line here.
    
    These should use the from * import * form.
  34. rbtools/tests.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  35. 
      
MB
chipx86
  1. This is looking great! A few small things left.
    
    A couple things I just want to check. These are up-to-date with the clients on master, right? (I don't believe we've touched them since you started, just want to make sure you synced them when you did). Also, is pyflakes still happy?
  2. rbtools/clients/__init__.py (Diff revision 2)
     
     
     
     
    Show all issues
    Two blank lines.
  3. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Can you change this to logging.error?
  4. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues
    Not your fault, but while you're here, "Repository's"
  5. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
     
     
    Show all issues
    Can you fix this indentation?
  6. rbtools/clients/client.py (Diff revision 2)
     
     
    Show all issues
    I think the contents of this file should also go in clients/__init__.py.
    
    That way, clients is the base module for what you need to make a client, rather than it being a sub-module of it.
  7. rbtools/clients/cvs.py (Diff revision 2)
     
     
    Show all issues
    logging.error
  8. rbtools/clients/perforce.py (Diff revision 2)
     
     
    Show all issues
    logging.info
  9. rbtools/postreview.py (Diff revision 2)
     
     
     
     
    Show all issues
    The first rbtools.clients should be grouped with the others, in alphabetical order.
  10. rbtools/postreview.py (Diff revision 2)
     
     
     
    What are we still importing these for?
    1. Which one? Left snippet imports json_loads which is used at 675 and 688 and the right one imports client classes of Perforce and Plastic SCMs and these are used too.
  11. rbtools/tests.py (Diff revision 2)
     
     
     
    You can import both on the same line.
  12. rbtools/tests.py (Diff revision 2)
     
     
    I would import postreview above.
    
    from rbtools import postreview
    
    Then it's just a matter of accessing postreview.options.
  13. rbtools/utils/process.py (Diff revision 2)
     
     
    Show all issues
    Import only what you need.
  14. rbtools/utils/process.py (Diff revision 2)
     
     
    Show all issues
    Should be two blank lines before this, rather than one.
  15. 
      
MB
chipx86
  1. Almost there!
  2. rbtools/clients/clearcase.py (Diff revisions 2 - 3)
     
     
    Should fit in < 80 chars.
  3. rbtools/tests.py (Diff revisions 2 - 3)
     
     
    Show all issues
    This should go before the others. Shallow depth first.
  4. rbtools/utils/process.py (Diff revisions 2 - 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Actually, importing a global is kinda bad. Instead, I'd add a new clean_tempfiles function in rbtools.utils.files, which die() can call.
  5. rbtools/clients/clearcase.py (Diff revision 3)
     
     
    Show all issues
    Same here.
    
    All other files too.
  6. rbtools/tests.py (Diff revision 3)
     
     
    Show all issues
    RepositoryInfo moved, so this shouldn't still work.
    
    Make sure you've deleted the client.pyc file and re-run both pyflakes and the unit tests.
  7. rbtools/utils/checks.py (Diff revision 3)
     
     
     
     
    Show all issues
    Two blank lines.
  8. 
      
MB
chipx86
  1. 
      
  2. rbtools/clients/__init__.py (Diff revisions 3 - 4)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    So this will actually set the local variable and not the global one. Did you test post-review after this change?
    
    You can use the global statement, like so:
    
    def load_scmclients():
        global SCMCLIENTS
        from ....
    
        SCMCLIENTS = [
            ...
        ]
    1. Sure, I've run them, but it seems that auto-deduction of SCM tool is not tested at all.
  3. 
      
MB
chipx86
  1. This looks good but no longer applies. I think some changes were submitted the Subversion the other day. Can you make sure those are applied and then merge with master?
  2. 
      
MB
chipx86
  1. Ship It!
  2. 
      
MB
Review request changed
Status:
Completed
Change Summary:
Committed to master (83ecd70)