Code review comment for lp:~rvb/maas/cluster-own-ui4

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

The change in the man page is a good catch! I just wrote that, and I hadn't made the connection. In general, thanks for the changes you've made. I know I'm being difficult as always. :) I'll make up for that by letting you have your way with the en-masse accept/reject actions.

In no-bootimages-warning.html, pre-existing text says “cluster's listing page” — should that apostrophe be there?

Could you give make_title_entry a quick docstring? Doesn't have to be much, just enough to place it in context.

In the ‘post’ method, maybe move the comments about what case each ‘if’ block handles, into the block? You'd get something like:

        if 'mass_accept_submit' in request.POST:
            # Accept clusters en masse.
            number = NodeGroup.objects.accept_all_pending()
            messages.info(request, "Accepted %d cluster(s)." % number)
            return HttpResponseRedirect(reverse('cluster-list'))

This way the if/elif/else structure comes out more clearly, because those statements are the only thing at their indentation level. It also means that the reader doesn't have to add a mental “if applicable” proviso to each comment. Once you're past the ‘if,’ the situation is clear and unambiguous.

Finally, I don't actually see the unit tests for make_title_entry. Missing commit? Not pushed? Diff not updated..?

review: Approve

« Back to merge proposal