[sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function

[Jameson "Chema" Quinn]

Some quick responses, before I dig into this...

On Sun, Jun 8, 2008 at 4:08 AM, Marco Pesenti Gritti <mpgritti at gmail.com>
wrote:

> -import subprocess
> +from subprocess import Popen, PIPE
>  import re
>  import gettext
>  from optparse import OptionParser
> +import warnings
> +import subprocess
>
> warnings seem unused, doesn't pylint warn you about it?


Brain fart. I read pylint output, said "OK, there are warnings about unused
imports, but which unused imports?" :)  Actually, this is fixed in later
patches I sent. Will fix.


>
> Since you are importing subprocess just use that for Popen and PIPE.
>
> +from sugar.bundle.activitybundle import ActivityBundle, MANIFEST,
> list_files
>
> It doesn't make sense for activitybundle to expose something a generic
> as list_files, especially since you are just using it to walk files
> inside that module.
>

So I should duplicate the code of list_files? (This is my biggest question
with your response)


>
> I think the MANIFEST definition is overkill and you are using it
> inconsistently anyway. Just use the string.


As the number of metadata files in the activity format grows, it seems
likely that eventually we'll give them their own directory. Making this a
global now is the first step to being able to change it later. But if you
prefer, I'll use a string for now.


>
> +    def __init__(self, bundle_name, source_dir=None, dist_dir = None,
> +                 dist_name = None):
>
> As I explained in my previous mail, I want to keep passing in the
> config here. Please remove the extra params.


disagree a bit, but will do.


>
>
> +    def fix_manifest(self):
> +        allfiles =  list_files(self.config.source_dir,
> +                          ignore_dirs=['dist', '.git'],
> +                          ignore_files=['.gitignore', 'MANIFEST',
> +                                        '*.pyc', '*~', '*.bak'])
> +        for afile in allfiles:
> +            if afile not in self.config.bundle.manifest:
> +                self.config.bundle.manifest.append(afile)
> +        manifestfile = open(os.path.join(self.config.source_dir,
> +                                         MANIFEST),
> +                            "wb")
> +        for line in self.config.bundle.manifest:
> +            manifestfile.write(line + "\n")
>
> This is all really hard to read. Some suggestions:
>
> * Split the ignore lists out of the list_files call.
> * We are never using aX for list iteration in the code. Just use f or
> filename there.
> * Do a manifest = self.config.bundle.manifest, you are repeating it 3 times
> * s/manifestfile/manifest so that it feet in one line and you don't
> need the crazy split up.
> * Add a \n after "manifestfile = open..."


will do


>
>
>     def get_files(self):
> -        return list_files(self.config.source_dir,
> -                          ignore_dirs=['locale', 'dist', '.git'],
> -                          ignore_files=['.gitignore'])
> +        git_ls = Popen('git-ls-files', stdout=PIPE,
> cwd=self.config.source_dir)
> +        if git_ls.wait():
> +            #non-0 return code - failed
> +            return BuildPackager.get_files(self)
> +        f = git_ls.stdout
> +        files = []
> +        for line in f.readlines():
> +            filename = line.strip()
> +            if not filename.startswith('.'):
> +                files.append(filename)
> +        f.close()
> +        return files
>
> Please separate groups of code with \n to make it more readable. I
> don't think this comment is necessary "#non-0 return code - failed",
> it's obvious if you ever used Popen.
>

will do


>
> +        from sugar import activity
> +        from sugar import env
>
> Unless you have a concrete need for this please drop it from the
> patch. To fix it properly I think we need to really cleanup the
> dependencies. But that's another patch.
>

will do


>
> +    def _is_dir(self, filename):
> +    def _is_file(self, filename):
>
> As you did in your last patch remove the _
>

will do


>
> +    def install(self):
>
> I don't think installation should be expose by ActivityBundle, we need
> to cleanup the dependencies. Let's remove it from this patch and we
> can discuss how to do it properly
>

install was already exposed, I just refactored unpack out of it (and thus
also exposed unpack). This only showed up in the patch because I put unpack
first, and the body of unpack matched with that part of the body of install.
I'm pretty sure this function is used by Journal, we can't just drop it.


>
> +        try:
> +            f = self._get_file(MANIFEST)
> +        except IOError:
> +            f = None
> +        if not f:
> +            logging.warning("Activity directory lacks a MANIFEST file.")
> +            return []
>
> All the other code which uses _get_file, assumes it will just return
> None if there is an IOError. Seems easier to just change the method to
> really do so (if it doesn't already).


Will do.


>
>
> +        #strip trailing \n and other whitespace
>
> I think this comment is unnecessary, it's implicit in the definition
> of the strip function.
>

Will do.


>
> I have some more comments for the read_manifest implementation (as a
> start some \n will help readability), but I have to run out now. We
> can address those in the next iteration.
>

I'll start with the \n.


>
> Thanks!
> Marco
> _______________________________________________
> Sugar mailing list
> Sugar at lists.laptop.org
> http://lists.laptop.org/listinfo/sugar
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.laptop.org/pipermail/sugar/attachments/20080608/4790a7aa/attachment.html