[sugar] [PATCH] Refactor bundlebuilder

[Tomeu Vizoso]

Hi,

didn't found many interesting things, so I had to invent some new nitpicks:

    info_path = os.path.join(config.source_dir, 'activity', 'activity.info')
    f = open(info_path,'r')
    info = f.read()
    f.close()

    exp = re.compile('activity_version\s?=\s?([0-9]*)')
    match = re.search(exp, info)
    version = int(match.group(1)) + 1
    info = re.sub(exp, 'activity_version = %d' % version, info)

    f = open(info_path, 'w')
    f.write(info)
    f.close()

Why not using the ConfigParser to read and modify that file instead?

    retcode = subprocess.call(['git', 'commit', '-a', '-m % s' % changelog])

Surprised that '% s' % changelog works. May be more expected to use
'%s' instead?

    retcode = subprocess.call(['git', 'commit', '-a', '-m % s' % changelog])
    if retcode:
        print 'ERROR - cannot commit to git'

This will commit other commits that may be pending in the local tree.
Perhaps we should check at the start of this function with git-status
that no changes are pending? Somebody suggested that recently in some
other thread.

    retcode = subprocess.call(['git', 'push'])
    if retcode:
        print 'ERROR - cannot push to git'

Similarly, we may want to check before that the local tree is in sync
with the remote, so we don't push other commits.

    zf = zipfile.ZipFile(packager.package_path)

    for name in zf.namelist():
        full_path = os.path.join(path, name)
        if not os.path.exists(os.path.dirname(full_path)):
            os.makedirs(os.path.dirname(full_path))

        outfile = open(full_path, 'wb')
        outfile.write(zf.read(name))
        outfile.flush()
        outfile.close()

Why not just use unzip?

    try:
        os.symlink(config.source_dir, bundle_path)
    except OSError:
        if os.path.islink(bundle_path):
            print 'ERROR - The bundle has been already setup for development.'
        else:
            print 'ERROR - A bundle with the same name is already installed.'

Should we check that the error is in fact "[Errno 17] File exists" and
not something else (no permission, non-existent dir, etc)?

                          ignore_dirs=[ 'locale', 'dist', '.git' ],

What's this thing about spaces enclosing the elements of a list? ;)

    def package(self):


        tar = tarfile.open(self.package_path, "w")

Extra spaces.

# Copyright (C) 2006-2007 Red Hat, Inc.

We are in 2008!

Regards,

Tomeu