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

Revision history for this message
Jeroen T. Vermeulen (jtv) 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.

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

« Back to merge proposal