[pony-build] Review of max's env branch (was: Re: max's stuff)

[C. Titus Brown]

Hi Max,

ok, here's a whole raft of comments.

--

You shouldn't use docstrings for code comments, as you do before 'tmp_cache_dir
= ...'.  docstrings are meant to be used for computer-extractable help text.

You use the errno module to provide a more detailed error message when
you can't create the cache dir, but ... either way, it's an error, right?
Why do we care what type of error?  And shouldn't the whole function
fail if the directory doesn't exist and we can't create it?

The try/except should be around the os.mkdir statement only, not the whole
if/else, IMO.

Also, the name 'check_cache_dir' implies (to me) that it doesn't actually do
anything -- you should probably have it be something like 'create_cache_dir'.

--

In the constructor for VirtualenvContext, try to avoid shorthand like
'reqdependencies' (where 'req' stands for 'required').  Either write it
out longhand, 'required_dependencies', or just have it be 'required'
and 'optional'.  Clarity++ :)

When pip fails to install a required dependency, is there any way to have the
error message saved to whatever is reported to the server?

sys.exit is probably not the right thing to be calling - do you really want
to dump out of the script without giving it a chance to clean up the
tmp directory, etc?  What other options are there?

Please indent your comments in VirtualenvContext.initialize
(pony_client.py:224) properly.

--

Around line 440 (if os.path.isdir(repo_dir):) you should print something
out if you're going to clone from the central repo rather than updating
a local cache.

--

Overall, good work!  Let me know when you've had a chance to look these
comments over and address those that you think should be addressed...

cheers,
--titus
--
C. Titus Brown, ctb at msu.edu