Awesome will look it all over.<br><br>Virtualenv stuff is no where near reviewing, I was just messing around with a few things.<br><br>Max<br><br><div class="gmail_quote">On Mon, Feb 15, 2010 at 10:03 PM, C. Titus Brown <span dir="ltr"><<a href="mailto:ctb@msu.edu">ctb@msu.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Hi Max,<br>
<br>
ok, here's a whole raft of comments.<br>
<br>
--<br>
<br>
You shouldn't use docstrings for code comments, as you do before 'tmp_cache_dir<br>
= ...'. docstrings are meant to be used for computer-extractable help text.<br>
<br>
You use the errno module to provide a more detailed error message when<br>
you can't create the cache dir, but ... either way, it's an error, right?<br>
Why do we care what type of error? And shouldn't the whole function<br>
fail if the directory doesn't exist and we can't create it?<br>
<br>
The try/except should be around the os.mkdir statement only, not the whole<br>
if/else, IMO.<br>
<br>
Also, the name 'check_cache_dir' implies (to me) that it doesn't actually do<br>
anything -- you should probably have it be something like 'create_cache_dir'.<br>
<br>
--<br>
<br>
In the constructor for VirtualenvContext, try to avoid shorthand like<br>
'reqdependencies' (where 'req' stands for 'required'). Either write it<br>
out longhand, 'required_dependencies', or just have it be 'required'<br>
and 'optional'. Clarity++ :)<br>
<br>
When pip fails to install a required dependency, is there any way to have the<br>
error message saved to whatever is reported to the server?<br>
<br>
sys.exit is probably not the right thing to be calling - do you really want<br>
to dump out of the script without giving it a chance to clean up the<br>
tmp directory, etc? What other options are there?<br>
<br>
Please indent your comments in VirtualenvContext.initialize<br>
(pony_client.py:224) properly.<br>
<br>
--<br>
<br>
Around line 440 (if os.path.isdir(repo_dir):) you should print something<br>
out if you're going to clone from the central repo rather than updating<br>
a local cache.<br>
<br>
--<br>
<br>
Overall, good work! Let me know when you've had a chance to look these<br>
comments over and address those that you think should be addressed...<br>
<br>
cheers,<br>
--titus<br>
<font color="#888888">--<br>
C. Titus Brown, <a href="mailto:ctb@msu.edu">ctb@msu.edu</a><br>
</font></blockquote></div><br>