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

[Marco Pesenti Gritti]

On Sun, Jun 8, 2008 at 1:59 PM, Jameson Chema Quinn
<jquinn at cs.oberlin.edu> wrote:
>> 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)

Unless I'm missing something you won't really duplicate much of it,
since you are not using the ignore_* which are the big part of the
code and the actual purpose of that function. You will basically just
need the for...

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

If you move it to a directory you will better construct to the path
with os.path.join, so it would probably be a method of Bundle. Anyway
I don't think it's worth to worry about that yet, it's easy enough to
refactor if/when necessary.

>>
>>
>> +    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.

Just look at all the ugly None checks you need to do to support it... :)

>> +    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.

You are right, That's ok then.

Thanks,
Marco