Mario:

Nice work.

I've mostly tackled style nit-picks here, since I had my knuckles rapped for similar issues.

Basically it comes down to this:
-imports should be alphabetical (in fact, it seems to be:  if there's an unordered list of things, you might as well alphabetize)
-4 spaces per indentation, not 8.
-In most cases, pad if, if/else, and for blocks with spaces above and below.

Once those are fixed up, I'll take another look.

Thanks,

-Mike

Hm - I still think it's weird to have an array of single entry dicts like this.

Didn't we agree upon:

"database": {
  "mysql": {
   ...
   }
  "pqsql": {
   ...
   }
   ...etc?
}

From what I gather, we want these in alphabetical order, so:

import os
import platform
...

Other style nitpick - we want 4 spaces per indentation, not 8.

typo: "an" -> "and"

space after this

space before this

space before this

space after this

space before this

space before this

space before and after this

space before and after this

space before this

space before this

Personally I put them in order of lowest level to highest level, so common stuff at the top, and more specialized stuff at the bottom .. old habit .. it also happily turns out to make it look nice line length wise :)

import os
import sys
import shutil
import platform
import simplejson as json
from types import ListType
from subprocess import Popen, PIPE

But really no-one cares .. it's just whatever you're comfortable with and whatever makes it easy to read later; alphabetical is good too.

According to good old guido's python style guide, class is preceded with two blank lines. That also goes for tabs characters, they should all be 4 spaces I reckon:

http://www.python.org/dev/peps/pep-0008/

'file' is a reserved keyword. I wouldn't use it for variable nor attribute names myself.

Also I see that the file is actually a filename so probably I'd call it fileName :) 

No need for the brackets there:

self.arch, self.distro = self._get_osarch()

is fine

No need for the i = 0 nor i = i + 1. Just use enumerate:

for i, o in enumerate(options):
    ...

does the same thing

Use += ..

opt_str += "[%d] %s\n" % (i, o)

is the same result

Another good += candidate

Personally I don't like the redundant use of the valid variable .. it might be OK with some people, but I'd prefer something like:

while 1:
   do stuff
   if done:
       break

sort of style

I'd add a try..except around the 'int' conversion, and only need to convert it once .. maybe something like:

            try:
                choice = int(choice)
            except:
                continue  # Basically the choice is not an int, just ignore it .. probably should print some insult or something :P
			if choice in range(0, len(options)):
				break
		return choice

evil line with whitespace and nothing on it  EVIL!

use .items() instead of .keys():

for key, pkg in depmap.items():

You can just add lists together, no need for the second loop. Either:

options += opt.keys()

or:

options.extend(opt.keys())

This is just another option, I like this style myself, but some may see it as a bit freaky:


                descr = {
                    'database': 'Choose database:',
                    'web_server': 'Choose web server:'
                }.get(key, 'Not set!')

re-route not reroot.. haha, I bet I'm starting to annoy now

You can use a list comprehension here:

pkg_list = [pkg['pkg_name'] for pkg in self.pkgmgr_list]

self._source_install() already loops through self.source_list(), no need to call it once for every item.

Might want to capture stderr too here .. just incase

I'd use a try: finally: block here

        oldDir = os.getcwd()
		os.mkdir('/tmp/reviewboard_installer')
        try:

...


        finally:
            os.chdir(oldDir)
            shutil.rmtree('/tmp/reviewboard_installer')

I'm not exactly sure what the semantics of "an array of dicts of a single element" are here. Is there a particular meaning?

If not, I'd like to see it just be one dict:

{
    "mysql" : { ... },
    "pgsql" : { ... },
    ...
}

Indent is 4 spaces.

You don't need the parens here.

Can you catch only ValueError?

Since you're doing a numeric comparison, it'd be a lot easier to use >= 0 and < len(options).

The way you've written this it's generating a list and then checking every element.

I wonder if instead of having 3 lists and checking the string for each one, if it might be cleaner to have a dict of method name : list

{
    'pkgmgr': [],
    'easy_install': [],
    'source': []
}

and then you could just do self.pkgs[method].append(pkg)

I'm not sure it's valid to assume that 64-bit == x86_64

After this, you should probably check p1.returncode to make sure that it returned 0 and not an error.

Same here.

And again...

Can you explicitly pass in the mode as 0700? By default os.mkdir() uses 0777, which is a pretty big security risk when this is (presumably) running as a privileged user.

Hmm, so your installer has a dependency on wget? And it's assuming that everything's a .tar.gz?

class File1(object):

I'd just add a function comment in here:

"""Loads our settings from the provided json config file"""

Redundant function ?

redundant function ?

It'd be nice to have a nice comment here explaining what the function does, perhaps with an example output too..

You can get rid of the extra 'i' variable by using enumerate:


            default_num = -1
            for (k, v), i in self.depmap[cat]['options'].items():
                opt_str += "[%d] %s: %s\n" % (i, v['name'], v['description'])
                if k == self.depmap[cat]['default']:
                    default_num = i

I'd probably go with:

choice = raw_input(opt_str).strip()

Just to remove extra spaces

In my code I make all top level classes children of object:

class File2(object):

This makes it a 'new style class' rather than an 'old style class'

http://www.python.org/doc/newstyle/

Function comments should be in the form of a string. That way autodoc tools like Sphinx pick it up well. And they can compile all of your todos into a single page and cool stuff like that

evil trailing whitespace

dep = self.depmap[pkg]
pkgver = dep['default']
pkgdata = dep[pkgver]
method = pkgdata['method']
if method == 'pkgmgr':
...
# Just less lookups .. nit picking .. sorry

If you wanted to be really pythony, you could switch the if statements to:

myList = {'pkgmgr': self.pkgmgr_list, 'easy_install', self.easy_list}.get(method, self.source_list)
myList.append(pkg)

in fact it might be a neat idea to init that dictionary of list names to actual lists in the __init__:

self.name2list = {'pkgmgr': self.pkgmgr_list, 'easy_install', self.easy_list}

then you could use it where ever with:

myList = self.name2list.get(methodName, self.source_list)

I probably would move the file reading out to a 'process' or 'read' method .. just a personal preference, and keeps with how most python classes are .. you init a 'reader' object, then you fill with some file..

This whole block looks similar to above, I'm sure you could probably merge them into a single reusable func. Possibly a base class or utility class sort of thing

could be done with enumerate as above

for (k, v), i in enumerate(self.depmap.items()):
...

either:

deps = self.depmap[self.version]['dependencies']['requirements'][:]

or:

deps = []
deps.extend(self.depmap[self.version]['dependencies']['requirements'])

deps.extend(self.depmap[self.version]['dependencies']['optional'])

or possibly:

deps += self.depmap[self.version]['dependencies']['optional']