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.

No blank line.

Might as well call tempfile.mkstemp instead.

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.

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.

Should be a from/import.

While you're here, please change this to:

super(ClearCaseClient, self).__init__()

Two blank lines.

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.

Two blank lines.

Inherit from object.

This should be able to just use debug(), which can be in the utils file. Same with all others.

This should be in postreview.

Two blank lines.

This should be in postreview.

Two blank lines.

This should be in postreview.

Should use super.

Indentation is wrong, but will be fixed when calling execute directly and not self.util.execute.

These shouldn't have this alignment.

Two blank lines.

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.

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.

super

super

super

super

One blank line.

super

Imports should always be in alphabetical order.

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.

< 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): ?

Alphabetical order, and no blank lines between any of these.

No blank line here.

These should use the from * import * form.

Two blank lines.

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?

Two blank lines.

Can you change this to logging.error?

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

Can you fix this indentation?

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.

logging.error

logging.info

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

What are we still importing these for?

You can import both on the same line.

I would import postreview above.

from rbtools import postreview

Then it's just a matter of accessing postreview.options.

Import only what you need.

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

Almost there!

Should fit in < 80 chars.

This should go before the others. Shallow depth first.

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

Same here.

All other files too.

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.

Two blank lines.

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 = [
        ...
    ]

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?

Ship It!