Code review comment for lp:~rvb/maas/default-zone

Revision history for this message
Raphaël Badin (rvb) wrote :

> Having an entire "Is this the default zone" column with a "Yes" in exactly one
> row seems a bit officious. Normally I'd say: just append "(default)" to the
> name. But with the default zone's name fixed to "default," that would look
> ridiculous as well. How about we just leave it out? We could make the
> default zone look different by italicising its displayed name.

Yeah, good point. I tried italicising the name of the default zone but it really didn't look right. I think it's fine: it's named default and it doesn't have the icons to edit and remove it at the end of the line so it's pretty clear what it is and that it's special.

> Similarly, it looks a bit... clerical to say "Default / No" on every zone's
> details page and "Default / Yes" on the default zone's. Shouldn't we just
> have a notice like "This is the default zone. It cannot be renamed or
> deleted." on the default zone's page, and nothing on the rest? There may not
> even be a way to click through to that page without first noticing that there
> is a default zone.

True, done.

> Typo — test_updates_default_zone_description_works should lose either the
> "_works" suffix, of the first "s".

Done.

> Finally, several things about test_rejects_deletion_of_default_zone:
[..]
>

Fixed.

> If you want to verify that we actually got the ValidationError, use
> ExpectedException:
>
> with ExpectedException(ValidationError):
> self.client.post(
> ...)
>
> This will make the test fail if the expected exception is not raised.
> Arguably that makes the test more brittle if we ever abolish the
> ValidationError, but in that case the comments will need revisiting anyway so
> it's appropriate for the test to call attention to itself.

Well, I think it's a bit strange to code a test for something that is actually wrong so I left the try/except statement.

> Julian has been saying he would like us to use TODO markers only for reminders
> to ourselves, and never land them. It may be a little late for that though.

Okay, I've removed this TODO.

Thanks for the review!

« Back to merge proposal