<br><br><div class="gmail_quote">On Sun, Jun 8, 2008 at 6:27 PM, 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;">
Some quic<br>
<br>
* The patch does not apply cleanly to sugar-toolkit master.</blockquote><div><br>I'll be more careful with my git diff.<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>
* Do you actually need to move the imports in activitybundle for<br>
Develop or is it just a cleanup?</blockquote><div><br>No, removed.<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>
+IGNORE_DIRS=['dist', '.git'],<br>
+IGNORE_FILES=['.gitignore', 'MANIFEST', '*.pyc', '*~', '*.bak']<br>
+<br>
<br>
Keep these in the class they are used by.<br>
</blockquote><div><br>check<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>
+<br>
<br>
You are adding spaces there, please remove.<br>
<div class="Ih2E3d"><br>
for f in files:<br>
- if ignore_files and f not in ignore_files:<br>
</div>+ if not (ignore_files and<br>
<div class="Ih2E3d">+ [True for pat in ignore_files if fnmatch(f,pat)]):<br>
</div>+ #not (result matches a pattern in ignore_files, ignore it)<br>
<br>
This was really hard to parse for me. It should be simpler if you just<br>
build the list of files with something like "files = [n for n in names<br>
if fnmatch(n, pattern)]" and then iterate over it.</blockquote><div><br>check. I could also change the "for ..:.. append" to a "..+= [..for..] if you want<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>
dirs.remove(ignore)<br>
-<br>
return result<br>
<br>
</div>No need to remove the \n there. </blockquote><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, source_dir=None, dist_path = ""):<br>
<br>
The dist_path split is really confusing, better to pass in both a<br>
dist_dir and xo_name.<br>
In the implementation please use an if instead of the or (like I did<br>
for source_dir). The or is more compact but imo makes the thing harder<br>
to read.</blockquote><div><br>check.<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>
+ #list files<br>
+ allfiles = list_files(source_dir, ignore_dirs=IGNORE_DIRS,<br>
+ ignore_files=IGNORE_FILES)<br>
+<br>
+ #add them to internal copy of MANIFEST<br>
+ #note: duplicates/invalids already removed when creating bundle object<br>
+ for path in allfiles:<br>
+ if path not in manifest:<br>
+ manifest.append(path)<br>
+<br>
+ #write internal copy to disk<br>
+ manifestfile = open(os.path.join(source_dir,"MANIFEST"),"wb")<br>
+ for line in manifest:<br>
+ manifestfile.write(line + "\n")<br>
<br>
All the comments in this block are redundant, the code is clear enough.</blockquote><div><br>The thing about duplicate/invalid actually matters, leaving just that.<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>
+ if git_ls.wait():<br>
+ #fall back to MANIFEST<br>
<div class="Ih2E3d">+ return BuildPackager.get_files(self)<br>
<br>
</div>Fallback to MANIFEST doesn't make sense. We should fallback to the<br>
list_files which you are removing instead.</blockquote><div><br>check<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>
+ #cleanup and return<br>
<br>
Please avoid this kind of comments, they don't add anything.</blockquote><div><br>check. <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>
+ return [file.strip() for file in files if not file.startswith('.')]<br>
<br>
What's the reason to skip .* ?</blockquote><div><br>This is the logic from the old version of bundlebuilder. No preference, myself. I suspect that it is for .gitignore .<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>
+ #we can either do<br>
+ #pyXXXXlint: disable-msg=W0201<br>
+ #disables "Attribute %r defined outside __init__" msg.<br>
+ #(remove XXXX to enable), or we can do:<br>
+ self.manifest = None #which is meaningless but shuts pylint up.<br>
+ self.read_manifest()<br>
<br>
Setting it to None is fine, remove the comments.</blockquote><div><br>I left just one comment.<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>
+ try:<br>
+ f = self.get_file("MANIFEST")<br>
+ except IOError:<br>
+ f = None<br>
+ if not f:<br>
+ logging.warning("Activity directory lacks a MANIFEST file.")<br>
+ return []<br>
<br>
</div>This should be a None check... see previous review for the reasoning.</blockquote><div><br>check.<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>
+ for num, line in enumerate(manifestlines):<br>
+ if line:<br>
<br>
s/manifestlines/lines, it's clear from the context.</blockquote><div><br>check.<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>
Can line ever be None there? I haven't checked by I'd not expect<br>
readlines() to return Nones.</blockquote><div><br>Not None; "", which is also false.<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>
+ #check...<br>
+ if line.endswith("/"):<br>
+ #...dirs...<br>
+ if not self.is_dir(line):<br>
+ manifestlines[num] = ""<br>
+ logging.warning("Bundle %s: invalid dir "<br>
<div class="Ih2E3d">+ "entry in MANIFEST: %s"<br>
+ %(self._name,line))<br>
<br>
</div>Do we require a trailing / for directories?<br>
<br>
+ #remove trailing newlines - unlike internal newlines,<br>
<div class="Ih2E3d">+ # they do not help keep absolute position<br>
</div>+ while manifestlines and manifestlines[-1] == "":<br>
<div class="Ih2E3d">+ manifestlines = manifestlines[:-1]<br>
+ self.manifest = manifestlines<br>
<br>
</div>Can you explain the absolute position comment?</blockquote><div><br>It is for version control.<br>Version x: ["activity/<a href="http://activity.info">activity.info</a>", "main.py", "extra.py", "sillyname.py"]<br>
...<br>rm extra.py; mv sillyname.py bettername.py<br>...<br>version x+1: ["activity/<a href="http://activity.info">activity.info</a>", "main.py", "", "bettername.py"]<br><br>thus the "" in the middle is useful. "" on the end, however, is not.<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 unpack(self, install_dir, strict_manifest=False):<br>
<br>
What is the use case of strict_manifest?</blockquote><div><br>Intended to be turned on by default later, once current activities are fixed. It will encourages proper use of MANIFEST. The principle is SPOT - single point of truth - and it will reduce the amount of fix_manifest I have to do in Develop to avoid data loss. fix_manifest too often defeats the purpose of MANIFEST, we might as well be including implicitly.<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>
+ #check installed files against the MANIFEST<br>
<div class="Ih2E3d">+ manifestfiles = self.get_files(self._raw_manifest())<br>
</div>+ paths = []<br>
+ for root, dirs, files in os.walk(install_path):<br>
+ for f in files:<br>
<div class="Ih2E3d">+ rel_path = root[len(base_dir) + 1:]<br>
</div>+ paths.append(os.path.join(rel_path, f))<br>
+ for path in paths:<br>
+ if path in manifestfiles:<br>
+ manifestfiles.remove(path)<br>
+ elif path != "MANIFEST":<br>
+ logging.warning("Bundle %s: %s not in MANIFEST"%<br>
+ (self._name,path))<br>
+ if strict_manifest:<br>
+ os.remove(os.path.join(install_path, path))<br>
<br>
Some \n please :)<br>
<br>
+ #create empty directories<br>
+ for adir in self.get_dirs():<br>
+ dirpath = os.path.join(install_path, adir)<br>
+ if os.path.isdir(dirpath):<br>
+ logging.warning("Bunldle %s: non-empty dir %s in MANIFEST"%<br>
+ (self._name,dirpath))<br>
+ else:<br>
+ os.makedirs(dirpath)<br>
<br>
Can you explain the reason of this?</blockquote><div><br>On second thought, there is no good reason. Removed.<br><br>> Also, you left this in.<br>
> <br>> + print path<br><br>fixed<br><br>Here is the new version of the patch. Tell me if there are any problems applying it cleanly, and I will fix.<br><br>Jameson<br></div></div><br>