Code review comment for lp:~frankban/lpsetup/bug-1034602-handle-target-dir

* typo: does not exist

* It is minor but I'd change "the target dir {0} is not a valid Launchpad branch" to say it is not a valid Launchpad tree. It may be a valid branch but not have a tree associated with it, i.e. devel vs sandbox.

* I think it would be cleaner to move the addCleanup in HandleTargetDirTest.setUp to be done right after the mktemp().

* Your test rely on the supposition that mktemp() will put things in /tmp. It might be more self-documenting and future proof if you extracted the actual directory using os.path.dirname and use that instead.

* The test_exists test is a bit fragile in that other processes could create that same directory after you remove it. Highly unlikely. But, you could avoid the appearance of a problem by using a subdirectory inside temporary directory as something you know does not exist. Picky, for sure.

* In test_is_writable your cleanup is either unnecessary because rmtree doesn't care and removes it anyway or it depends on the chmod cleanup being called before the rmtree cleanup. I just verified it is the former, rmtree does not care about write permissions when removing an owned tree, so the chmod cleanup is unnecessary.

This is a great improvement. Thanks.