[Python-Dev] PEP 3144: IP Address Manipulation Library for the Python Standard Library

[Nick Coghlan]

Peter Moody wrote:
> On Thu, Aug 20, 2009 at 10:15 PM, Case Vanhorsen<casevh at gmail.com> wrote:
>> I was surprised that IP('172.16.1.1') returned
>> IPv4Address('172.16.1.1/32') instead of IPv4Address('172.16.1.1'). I
>> know I can change the behavior by using host=True, but then
>> IP('172.16.1.1/24', host=True) will raise an error. It makes more
>> sense, at least to me, that if I input just an IP address, I get an IP
>> address back. I would prefer that IP('172.16.1.1/32') return an
>> IPv4Network and IP('172.16.1.1') return an IPv4Address.
> 
> I think you mean that it returns an IPv4Network object (not
> IPv4Address).  My suggestion there is that if you know you're dealing
> with an address, use one of the IPvXAddress classes (or pass host=True
> to the IP function). IP is just a helper function and defaulting to a
> network with a /32 prefix seems relatively common.
> 
> Knowing that my experience may not always be the most common, I can
> change this behavior if it's indeed confusing, but in my conversations
> with others and in checking out the current state of ip address
> libraries, this seems to be a perfectly acceptable default.

The IP() helper function actually bothers me a bit - it's a function
where the return *type* depends on the parameter *value*. While
switching between IPv4 and IPv6 based on value is probably a necessary
behaviour, perhaps it would be possible to get rid of the "host=True"
ugliness and instead have two separate helper functions:

  IP() - returns either IPv4Address or IPv6Address
  IPNetwork() - returns either IPv4Network or IPv6Network

Both would still accept a version argument, allowing programmatic
control of which version to accept. If an unknown version is passed then
some kind of warning or error should be emitted rather than the current
silent fallback to attempting to guess the version based on the value.

I would suggest removing the corresponding IPv4 and IPv6 helper
functions altogether.

My rationale for the above is that hosts and networks are *not* the same
thing. For any given operation, the programmer should know whether they
want a host or a network and ask for whichever one they want. The
IPv4/IPv6 distinction, on the other hand, is something that a lot of
operations are going to be neutral about, so it makes sense to deal with
the difference implicitly.

Other general comments:

- the module appears to have quite a few isinstance() checks against
non-abstract base classes. Either these instance checks should all be
removed (relying on pure duck-typing instead) or else the relevant
classes should be turned into ABCs. (Note: this comment doesn't apply to
the type dispatch in the constructor methods)

- the reference implementation has aliased "CamelCase" names with the
heading "backwards compatibility". This is inappropriate for a standard
library submission (although I can see how it would be useful if you
were already using a different IP address library).

- isinstance() accepts a tuple of types, so isinstance(address, (int,
long)) is a shorter way of writing "isinstance(address, int) or
isinstance(address, long)". The former also has the virtue of executing
faster. However, an even better approach would be to use
operator.index() in order to accept all integral types rather than just
the builtin ones.

Cheers,
Nick.

-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia
---------------------------------------------------------------