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

[Max Laite]

Awesome will look it all over.

Virtualenv stuff is no where near reviewing, I was just messing around with
a few things.

Max

On Mon, Feb 15, 2010 at 10:03 PM, C. Titus Brown <ctb at msu.edu> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.idyll.org/pipermail/pony-build/attachments/20100215/7082325e/attachment.htm>