maas-devel team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

> [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.

It does something somewhat right now: it stores the date with the tz.  jtv would like all the dates to be stored without a timezone but I'll let him do it :).

> [2]
> 
> +def generate_node_system_id():
> +    return 'node-' + str(uuid1())
> 
>     return 'node-%s' % uuid1()
> 
> might be prettier :) No other reason.

Sure.

> [3]
> 
> +    def __unicode__(self):
> +        return self.system_id
> 
> Is this used by Django, or is it for your own information?

It is used by django when as the default __repr__ for instance when printing an object.  It is just convenient to have the system_id.

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

The default __repr__ is <Node Ox1234>.

> [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: (?:...)

I prefer the former.

> [5]
> 
> +        self.assertTrue(node.system_id.startswith('node-'))
> 
> Perhaps this is the time to introduce testtools and use StartsWith?

ah! I'll skip that one for now.

> [6]
> 
> Docstrings needed!

Docstrings added.

> 
> 
> [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?

That's the way it is, the validation happens when you call full_clean on the model (see https://docs.djangoproject.com/en/dev/ref/models/instances/#validating-objects).
-- 
https://code.launchpad.net/~rvb/maas/maas-basic-model/+merge/88890
Your team MaaS Developers is subscribed to branch lp:maas.