Comment 3 for bug 208606

Hi Andrew,

Andrew Hunter wrote:
 > Then we need to move the warning in text.py and remove it from there.

I just had a peek at text.py and I'm not sure I follow. At this stage
it's probably fine to let any exceptions bubble up and blow up. At
some point it'd be nice to have some top-level exception handling that
presents errors to users in a nice way.

One of the things that hasn't been entirely clear to me is what the
best way to do this is. One the one hand, it's not great to let
errors blow up the application because it's (1) an ugly user interface
for users and (2) can leave cruft lying around, depending on where the
error occurs. On the other hand, I've actually wanted to have things
left in whatever state they are when problems happen so that I can
debug issues. Perhaps the right thing is to have both behaviours: we
clean up in the normal case and leave cruft lying around when (one of)
the --no-cleanup option is present or when an AUTOPPA_DEBUG
environment variable is defined.

 > I did this because os.makedirs() raises an OSError if the specified
 > directory already exists. Suggestions?

Ah, okay. I'd forgotten that detail about os.makedirs. I recommend
something like this:

if not os.path.exists(path):
     os.makedirs(path)

If the path already exists we should issue a warning, remove the
existing path and continue. I'm a little worried about the potential
to destroy user's data if, for example, they have put the wrong path
in ~/.autoppa.conf. A part of me thinks it might be better to bail
with a warning like:

The working directory $path already exists. Please remove this before
running AutoPPA again.

 > For the commit() call, I am specifying it by reading the file, but
 > re-reading the code, it makes more sense to just close the file and use
 > the changelog varriable in the commit. I'll fix that.

I think the using the changelog variable is the right way to go here,
too.

 > A Vcs system by their very nature log everything they do. Should not be
 > too difficult to use some of python's built in logging module to keep a
 > record of each action taken. I'll create a TODO list with these things
 > on it.

Yeah, that's what I figured. I'm not very familiar with bzrlib, so
this will require some investigation. Given that pexpect does it's
own logging, we'll have to look at changing existing things to use the
logging module.

 > Ok cool, I have never worked with self-testing in a codebase before
 > (self-taught programmer), but I can see the huge advantages of that. I
 > do the reading and fix the code.

It's the best way I know of to maintain production quality in a
codebase. The combination of having a rigourous test suite and
requiring code reviews of every branch before code is merged to trunk
works well to ensure that trunk is always stable. In fact, on the
projects I work on at Canonical (Landscape, Storm and AutoPPA) we
require two reviews per branch before merging to ensure that whatever
lands in trunk is stable and production-ready. If you have questions
about how to do TDD, or about any aspects of the testing
infrastructure please ask.

Thanks!