Some quick responses, before I dig into this...<br><br><div class="gmail_quote">On Sun, Jun 8, 2008 at 4:08 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;">
<div class="Ih2E3d">-import subprocess<br>
+from subprocess import Popen, PIPE<br>
import re<br>
import gettext<br>
from optparse import OptionParser<br>
+import warnings<br>
+import subprocess<br>
<br>
</div>warnings seem unused, doesn't pylint warn you about it?</blockquote><div><br>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.<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
Since you are importing subprocess just use that for Popen and PIPE.<br>
<div class="Ih2E3d"><br>
+from sugar.bundle.activitybundle import ActivityBundle, MANIFEST, list_files<br>
<br>
</div>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>
</blockquote><div><br>So I should duplicate the code of list_files? (This is my biggest question with your response)<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
I think the MANIFEST definition is overkill and you are using it<br>
inconsistently anyway. Just use the string.</blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><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.</blockquote><div><br>disagree a bit, but will do.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
+ def fix_manifest(self):<br>
+ allfiles = list_files(self.config.source_dir,<br>
+ ignore_dirs=['dist', '.git'],<br>
<div class="Ih2E3d">+ ignore_files=['.gitignore', 'MANIFEST',<br>
+ '*.pyc', '*~', '*.bak'])<br>
</div>+ for afile in allfiles:<br>
+ if afile not in self.config.bundle.manifest:<br>
+ self.config.bundle.manifest.append(afile)<br>
+ manifestfile = open(os.path.join(self.config.source_dir,<br>
<div class="Ih2E3d">+ MANIFEST),<br>
+ "wb")<br>
+ for line in self.config.bundle.manifest:<br>
+ manifestfile.write(line + "\n")<br>
<br>
</div>This is all really hard to read. Some suggestions:<br>
<br>
* Split the ignore lists out of the list_files call.<br>
* We are never using aX for list iteration in the code. Just use f or<br>
filename there.<br>
* Do a manifest = self.config.bundle.manifest, you are repeating it 3 times<br>
* s/manifestfile/manifest so that it feet in one line and you don't<br>
need the crazy split up.<br>
* Add a \n after "manifestfile = open..."</blockquote><div><br>will do<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<div class="Ih2E3d"><br>
def get_files(self):<br>
- return list_files(self.config.source_dir,<br>
- ignore_dirs=['locale', 'dist', '.git'],<br>
- ignore_files=['.gitignore'])<br>
+ git_ls = Popen('git-ls-files', stdout=PIPE, cwd=self.config.source_dir)<br>
+ if git_ls.wait():<br>
+ #non-0 return code - failed<br>
</div>+ return BuildPackager.get_files(self)<br>
<div class="Ih2E3d">+ f = git_ls.stdout<br>
+ files = []<br>
+ for line in f.readlines():<br>
+ filename = line.strip()<br>
+ if not filename.startswith('.'):<br>
+ files.append(filename)<br>
+ f.close()<br>
+ return files<br>
<br>
</div>Please separate groups of code with \n to make it more readable. I<br>
don't think this comment is necessary "#non-0 return code - failed",<br>
it's obvious if you ever used Popen.<br>
</blockquote><div><br>will do<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
+ from sugar import activity<br>
+ from sugar import env<br>
<br>
Unless you have a concrete need for this please drop it from the<br>
patch. To fix it properly I think we need to really cleanup the<br>
dependencies. But that's another patch.<br>
</blockquote><div><br>will do<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
+ def _is_dir(self, filename):<br>
+ def _is_file(self, filename):<br>
<br>
As you did in your last patch remove the _<br>
</blockquote><div><br>will do<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><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>
</blockquote><div><br>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.<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
+ try:<br>
<div class="Ih2E3d">+ f = self._get_file(MANIFEST)<br>
+ except IOError:<br>
</div>+ f = None<br>
+ if not f:<br>
+ logging.warning("Activity directory lacks a MANIFEST file.")<br>
+ return []<br>
<br>
All the other code which uses _get_file, assumes it will just return<br>
None if there is an IOError. Seems easier to just change the method to<br>
really do so (if it doesn't already).</blockquote><div><br>Will do.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<div class="Ih2E3d"><br>
+ #strip trailing \n and other whitespace<br>
<br>
</div>I think this comment is unnecessary, it's implicit in the definition<br>
of the strip function.<br>
</blockquote><div><br>Will do.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
I have some more comments for the read_manifest implementation (as a<br>
start some \n will help readability), but I have to run out now. We<br>
can address those in the next iteration.<br>
</blockquote><div><br>I'll start with the \n.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
Thanks!<br>
<div><div></div><div class="Wj3C7c">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>