maas-devel team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Review: Needs Information

Looks good. Lots of questions though :)


[1]

+    def save(self):
+        if not self.id:
+            self.created = datetime.date.today()
+        self.updated = datetime.datetime.today()

I assume this needs a sprinkling of pytz? Or does Django mysteriously
DTRT here? I think we should prefer explicit when dealing with dates.


[2]

+def generate_node_system_id():
+    return 'node-' + str(uuid1())

    return 'node-%s' % uuid1()

might be prettier :) No other reason.


[3]

+    def __unicode__(self):
+        return self.system_id

Is this used by Django, or is it for your own information?

Does Django provide a decent repr around this? In other words, it
would be nice to have a decent repr :)


[4]

+mac_re = re.compile(r'^([0-9a-fA-F]{2}(:|$)){6}$')

I'm not sure about the $ in the middle. It will also match
"12:34:56:78:90:12:" even though the max_length setting on mac_address
will catch that.

The following might be clearer:

mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')

Or even:

mac_re = re.compile(
   r"^ (?:[0-9a-fA-F]{2}:){5} [0-9a-fA-F]{2} $", re.VERBOSE)

Note also the non-capturing group I added: (?:...)


[5]

+        self.assertTrue(node.system_id.startswith('node-'))

Perhaps this is the time to introduce testtools and use StartsWith?


[6]

Docstrings needed!


[7]

+        mac = MACAddress(mac_address='AA:BB:CCXDD:EE:FF', node=node)
+        self.assertRaises(ValidationError, mac.full_clean)

Wait! What? It doesn't blow up until later?

-- 
https://code.launchpad.net/~rvb/maas/maas-basic-model/+merge/88890
Your team MaaS Developers is subscribed to branch lp:maas.