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

[Peter Moody]

On Fri, Aug 21, 2009 at 4:41 PM, Nick Coghlan<ncoghlan at gmail.com> wrote:
> 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

IPAddress() and IPNetwork seem a little less confusing. I'll get rid
of IP() since it seems to be the source of a fair bit of confusion.

I've added this to the pep3144 change. need to change the pep to
reflect this now.

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

done.

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

makes sense to me.

> 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)

I'll look through this.

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

this is for legacy reasons (how it's used at work). it's fully
expected that those will disappear if/when this is accepted.

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

I can easily make the change to checking tuples.  I'd have to look
further at the operator.index() to see what's required.

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