Merge lp://staging/~julian-edwards/maas/forwarders-config-writer into lp://staging/~maas-committers/maas/trunk

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.

.

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?

.

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 see no mention of what it means if you already had a ‘forwarders’ setting. And yet we overwrite it. Worth saying?

.

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?

.

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

.

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.

.

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?

.

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

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!

> > 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.

You did, however, write some top comments that explained what the methods were for — that's exactly what should be in a docstring.

> > 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.

That says it's an error to have more than one. But it doesn't say where that pre-existing one would come from, and why it is safe to delete it. Might cause future maintainers to miss subtleties.

On Tuesday 18 Feb 2014 09:20:13 you wrote:
> > > 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.
>
> You did, however, write some top comments that explained what the methods
> were for — that's exactly what should be in a docstring.

Oh sorry, I see what you mean now. Yes, they were left over from refactored
blocks, which now look like they ought to be docstrings. So now they are.

> > > 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.
>
> That says it's an error to have more than one. But it doesn't say where
> that pre-existing one would come from

Not sure how I could know that :)

>, and why it is safe to delete it.
> Might cause future maintainers to miss subtleties.

I've mentioned that the old value is going to be in the backup file made
(exactly why I was careful about making a backup).

cheers.