Code review comment for lp:~gmb/maas/bug-1336617

Revision history for this message
Gavin Panella (allenap) wrote :

>>> + (self.static_ip_range_low and self.static_ip_range_high)):
>>
>> Should do the `is not None` thing here, to avoid evaluating
>> static_ip_range_{low,high} in a boolean context, which might not
>> coincide with what you intend.
>
> No; (static_)ip_range_(low|high) can be an empty string, too.

If you wanted to be a really good citizen you should check `thing is not
None and len(thing) != 0` but I think you can be forgiven here because
Django.

>
>>> + errors = {}
>>> + ip_range_low = IPAddress(self.ip_range_low)
>>> + ip_range_high = IPAddress(self.ip_range_high)
>>> + static_ip_range_low = IPAddress(self.static_ip_range_low)
>>> + static_ip_range_high = IPAddress(self.static_ip_range_high)
>>> +
>>> + message_base = (
>>> + "Lower bound %s is higher than upper bound %s")
>>
>> Any reason we don't reorder them automatically?
>
> Yes. If I write:
>
> ip_range_low = 10.1.0.100
> ip_range_high = 10.1.0.15
>
> How are we to know that I didn't mean to write low = 10.1.0.10?

That's a good point.

« Back to merge proposal