Here is the revised patch. It has your suggested changes, plus a couple more:<br><br>- Check for existence of po directory in Builder<br>- Config.__init__() is cleaned up. Now gets bundle name from <a href="http://activity.info">activity.info</a>. start() no longer needs a bundle name, and has deprecation warning. Also I put things in a more logical order.<br>
<br><div class="gmail_quote">On Sun, Jun 8, 2008 at 7:11 AM, Marco Pesenti Gritti <<a href="mailto:mpgritti@gmail.com">mpgritti@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
On Sun, Jun 8, 2008 at 1:59 PM, Jameson Chema Quinn<br>
<div class="Ih2E3d"><<a href="mailto:jquinn@cs.oberlin.edu">jquinn@cs.oberlin.edu</a>> wrote:<br>
</div><div class="Ih2E3d">>> It doesn't make sense for activitybundle to expose something a generic<br>
>> as list_files, especially since you are just using it to walk files<br>
>> inside that module.<br>
><br>
> So I should duplicate the code of list_files? (This is my biggest question<br>
> with your response)<br>
<br>
</div>Unless I'm missing something you won't really duplicate much of it,<br>
since you are not using the ignore_* which are the big part of the<br>
code and the actual purpose of that function. You will basically just<br>
need the for...<br>
<div class="Ih2E3d"><br>
>><br>
>> I think the MANIFEST definition is overkill and you are using it<br>
>> inconsistently anyway. Just use the string.<br>
><br>
> As the number of metadata files in the activity format grows, it seems<br>
> likely that eventually we'll give them their own directory. Making this a<br>
> global now is the first step to being able to change it later. But if you<br>
> prefer, I'll use a string for now.<br>
<br>
</div>If you move it to a directory you will better construct to the path<br>
with os.path.join, so it would probably be a method of Bundle. Anyway<br>
I don't think it's worth to worry about that yet, it's easy enough to<br>
refactor if/when necessary.<br>
<div class="Ih2E3d"><br>
>><br>
>><br>
>> + def __init__(self, bundle_name, source_dir=None, dist_dir = None,<br>
>> + dist_name = None):<br>
>><br>
>> As I explained in my previous mail, I want to keep passing in the<br>
>> config here. Please remove the extra params.<br>
><br>
> disagree a bit, but will do.<br>
<br>
</div>Just look at all the ugly None checks you need to do to support it... :)<br>
<div class="Ih2E3d"><br>
>> + def install(self):<br>
>><br>
>> I don't think installation should be expose by ActivityBundle, we need<br>
>> to cleanup the dependencies. Let's remove it from this patch and we<br>
>> can discuss how to do it properly<br>
><br>
> install was already exposed, I just refactored unpack out of it (and thus<br>
> also exposed unpack). This only showed up in the patch because I put unpack<br>
> first, and the body of unpack matched with that part of the body of install.<br>
> I'm pretty sure this function is used by Journal, we can't just drop it.<br>
<br>
</div>You are right, That's ok then.<br>
<div><div></div><div class="Wj3C7c"><br>
Thanks,<br>
Marco<br>
_______________________________________________<br>
Sugar mailing list<br>
<a href="mailto:Sugar@lists.laptop.org">Sugar@lists.laptop.org</a><br>
<a href="http://lists.laptop.org/listinfo/sugar" target="_blank">http://lists.laptop.org/listinfo/sugar</a><br>
</div></div></blockquote></div><br>