launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

This test is ambiguous:

298	+    def test_manager_get_config_list_returns_config_list(self):
299	+        Config.objects.create(name='name', value='config1')
300	+        Config.objects.create(name='name', value='config2')
301	+        config_list = Config.objects.get_config_list('name')
302	+        self.assertSequenceEqual(['config1', 'config2'], config_list)

You test the exact order in which the items are retrieved, but you neither specify nor test whether that is alphabetical order, creation order, or some other order.

Moreover, the combination of "store lists as config values" and "store config values and I'll turn them into a list" seems destined to cause confusion, much like the trouble we had with Django's weird request parameter dicts.  What exactly is the overriding requirement?  Without one, I would say please keep it simple.  The need for this kind of thing would be useful to explain in your cover letter, i.e. the description on the MP.  But it's hard for me to judge without knowing more about the problem this branch solves.
-- 
https://code.launchpad.net/~rvb/maas/config-table/+merge/93861
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/config-table into lp:maas.