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">&lt;<a href="mailto:ctb@msu.edu">ctb@msu.edu</a>&gt;</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&#39;s a whole raft of comments.<br>
<br>
--<br>
<br>
You shouldn&#39;t use docstrings for code comments, as you do before &#39;tmp_cache_dir<br>
= ...&#39;.  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&#39;t create the cache dir, but ... either way, it&#39;s an error, right?<br>
Why do we care what type of error?  And shouldn&#39;t the whole function<br>
fail if the directory doesn&#39;t exist and we can&#39;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 &#39;check_cache_dir&#39; implies (to me) that it doesn&#39;t actually do<br>
anything -- you should probably have it be something like &#39;create_cache_dir&#39;.<br>
<br>
--<br>
<br>
In the constructor for VirtualenvContext, try to avoid shorthand like<br>
&#39;reqdependencies&#39; (where &#39;req&#39; stands for &#39;required&#39;).  Either write it<br>
out longhand, &#39;required_dependencies&#39;, or just have it be &#39;required&#39;<br>
and &#39;optional&#39;.  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&#39;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&#39;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>