I've addressed all comments, and done quite a bit of experimenting today to see
if there was a way to restructure this to use a contextmanager, as that has been
a persistent comment. In short, it cannot be done, but I directly address that
question in in-line reply.
https://codereview.appspot.com/7137054/diff/5002/charmworld/jobs/proof.py
File charmworld/jobs/proof.py (right):
https://codereview.appspot.com/7137054/diff/5002/charmworld/jobs/proof.py#new...
charmworld/jobs/proof.py:15: raise OSError('[Errno 2] No such file or
directory')
On 2013年01月22日 15:08:40, abentley wrote:
> Please use a different, perhaps custom exception. Saying "No such file" when
> there may be a file is confusing at best.
With your suggestion for moving unpatch to a separate try block, rather than the
previous try/except/else/finally, we can restructure this to simply return a
boolean rather than using the exception. I did still use your suggestion of
os.stat to ensure the path exists.
It should be noted, that I'm putting the unpatch call inside a try/finally,
rather than a try/except, as I don't want to handle silence errors that I don't
expect--I simply want to ensure unpatch is called regardless of the proofing
process.
https://codereview.appspot.com/7137054/diff/5002/charmworld/jobs/proof.py#new...
charmworld/jobs/proof.py:30: for l in lint:
On 2013年01月22日 15:08:40, abentley wrote:
> Please use more descriptive variable names, e.g. "line".
Done.
https://codereview.appspot.com/7137054/diff/5002/charmworld/jobs/proof.py#new...
charmworld/jobs/proof.py:42: try:
On 2013年01月22日 14:19:40, hazmat wrote:
> in general this sort of usage lock/unlock is perfect for a context manager.
the
> exception catch here for a list append feels a bit over much vs just using a
> boolean return value.
A try/finally is necessary to make certain we restore the path if we've altered
it; a context manager doesn't always execute the exit case in the case of an
unhandled exception.
> ideally this patch is also doing the import, so we can check/guard importerror
> and report in the same block. else with the current structure, the whole queue
> would be processed with errors even if we couldn't import the prooflib.
Because we can't safely restructure this with a context manager, we can't have
patch_path do the import, as the import is then lost on the return. However, we
can change it so that if patch fails, we cease execution of `run`, which ends
the proofing run.