[sugar] [PATCH] Refactor bundlebuilder

[Marco Pesenti Gritti]

On Tue, May 27, 2008 at 4:25 PM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
> 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.

I didn't touch cmd_release but I plan to just after I land this patch.
I'll keep your notes for then.

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

Old code. I can address it just after landing this one. Perhaps it
would make it less portable though?


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

Hmmm we can surely do better. But again it's old code, so I'll address
it with the next set of patches.

>
>                          ignore_dirs=[ 'locale', 'dist', '.git' ],
>
> What's this thing about spaces enclosing the elements of a list? ;)

Isn't it nice? :)

Ok, fixed...

>    def package(self):
>
>
>        tar = tarfile.open(self.package_path, "w")
>
> Extra spaces.

Ooops, fixed.

> # Copyright (C) 2006-2007 Red Hat, Inc.
>
> We are in 2008!

Fixed.

Thanks!
Marco