Code review comment for lp:~allenap/maas/repackage

> Plowing right on!  Looking forward to getting this in.

Cheers, thank you for persisting :)

>
> In pkg/maas-region/maasserver/components.py, line 42 or so, you have a
> docstring, then a function call, and *then* an import that's there to avoid an
> import cycle.  Would be more regular and obvious to have the import right
> below the docstring:
>
> 3328         :param error_message: Human-readable error text.
> 3329         """
> 3330         discard_persistent_error(component)
> 3331    +    from maasserver.models import ComponentError  # Avoid circular
> imports.
> 3332         ComponentError.objects.create(component=component,
> error=error_message)

I've changed it.

>
> .
>
> The comment in MAASServerTestCase.setUp really doesn't need to have a TODO
> marker as far as I'm concerned.  It's a great comment, but it does not instill
> a great sense of urgency.  :-)

Fixed :)

>
> .
>
> In pkg/maas-region/maasserver/tests/test_views_nodes.py, why does
> test_node_list_sorts_by_zone newly skip with an XXX message from gmb?  It
> looks as if you were just porting over a previous skip() to a better format,
> but I don't see anything that would have made that test skip before.

Ah, I think you're right. I think I changed it from using `raise
SkipTest(...)` to using `self.skip(...)` thus avoiding the import of
SkipTest. Then Raffolo fixed it, I merged trunk, but no conflict (or
easily overlooked conflict) arose.

>
> .
>
> The documentation for ClassSetup relies on knowledge of MAASTestCaseType in a
> different module.  Can you explain it in such a way that people who aren't
> aware of MAASTestCaseType's relevance have a chance of understanding it?  Can
> you also explain in docstrings *why* MAASTestCaseType forbids setUpClass if we
> then need ClassSetup to un-forbid it again?

Docstring updated with:

    `MAASTestCaseType` forbids `setUpClass` and `tearDownClass` in new
    test classes, but sometimes they must be called in existing super
    classes, one example being :class:`django.test.LiveServerTestCase`.