Code review comment for lp://staging/~julian-edwards/maas/forwarders-config-writer

Thank you for the review!

On Monday 17 Feb 2014 11:11:38 you wrote:
> Review: Approve
>
> Glad you got this to work. There's always a certain comfort about having a
> third party worry about a problem like this.
>
> Hang on.
>
> Hang on...
>
> Question: are we not writing docstrings anymore? That's fine, whatever.
> But if we're doing a new thing, nobody told me.

Write them judiciously. In other words, does it add value? If it does do it,
if not, don't.

Most of the functions here are what I factored out to make the handle() func
less of an eyesore, but it made the whole thing longer and harder to read.
*shrug*

> In parse_file, given the agoraphobiagenic broadness of the exceptions, could
> you at least report, in one way or another, the original exception message
> as raised by iscpy?

Ah yes, good point.

> In set_up_include_statement, I have a vague memory of there being a reason
> why neither os.path.join nor posixpath.join was appropriate here... worth a
> comment maybe?

I can't honestly remember either. The reason I did it the way I did was
partly for readability and partly because it's a text file not an argument to
a function that needs a path.

> I see no mention of what it means if you already had a ‘forwarders’ setting.
> And yet we overwrite it. Worth saying?

There's already a comment, did you miss it or did I not write enough?

        # Remove existing forwarders, it's a syntax error to have more
        # than one in the combined configuration for named.

> Why the vague implicit-bool-based test “options_block.get('forwarders',
> None)” in remove_forwarders? Are you saying that if there's an empty
> string rather than None, it should not be removed? Would "'forwarders' in
> options_block" not produce the right answer?

I've simplified it, thanks. Lyme brain... :(

> In back_up_existing_file, could you print/log the original exception message
> as raised from iscpy?

It's not raised from iscpy, but yes I am now printing the IOError message,
thanks for pointing it out.

> Moving on to the tests, test_exits_when_no_file_to_edit relies on there
> being no file called 'foo'. I think it would make more sense to do this in
> a temporary directory.

Indeed, fixed!

> In test_exits_when_fails_to_make_backup, is Mock not more convenient than
> defining a function? Also, isn't the realistic thing to do to raise an
> IOError object, rather than the IOError class?

I had forgotten about Mock.side_effect, so I wrote the func meaning to come
back to it later and.... forgot!

Also, yes, I need to raise an object, good spot.

> In test_normal_operation, would glob() be an easier way to get the backup
> file?

I did think about that originally but I have a vague aversion to using it in
tests as it means there's some indeterminate part somewhere. I think this way
is better as it knows we only have two files so if we remove the known one,
the remaining one must be the backup file.

Thanks again!