[Python-Dev] [Python-checkins] cpython: Issue #14814: addition of the ipaddress module (stage 1 - code and tests)

[Terry Reedy]

On 5/20/2012 7:02 AM, nick.coghlan wrote:


> +def ip_address(address, version=None):
> +    """Take an IP string/int and return an object of the correct type.
> +
> +    Args:
> +        address: A string or integer, the IP address.  Either IPv4 or
> +          IPv6 addresses may be supplied; integers less than 2**32 will
> +          be considered to be IPv4 by default.
> +        version: An Integer, 4 or 6. If set, don't try to automatically

integer, not Integer

> +          determine what the IP address type is. important for things
> +          like ip_address(1), which could be IPv4, '192.0.2.1',  or IPv6,
> +          '2001:db8::1'.

I read this as saying that a version other than 4 or 6 should be an 
error, and not ignored as if not set. If version is set incorrectly, it 
is still set. I certainly would expect an error to be an error.

> +
> +    Returns:
> +        An IPv4Address or IPv6Address object.
> +
> +    Raises:
> +        ValueError: if the string passed isn't either a v4 or a v6
> +          address.

Should say "if the *address*...", and I suggest adding "or if the 
version is not None, 4, or 6."
> +
> +    """
> +    if version:

if version is not None: ??
Do you really want to silently ignore *every* null value, like '' or []?


> +        if version == 4:
> +            return IPv4Address(address)
> +        elif version == 6:
> +            return IPv6Address(address)

else: raise ValueError() ??

...

> +def ip_network(address, version=None, strict=True):
> +    """Take an IP string/int and return an object of the correct type.
> +
> +    Args:
> +        address: A string or integer, the IP network.  Either IPv4 or
> +          IPv6 networks may be supplied; integers less than 2**32 will
> +          be considered to be IPv4 by default.
> +        version: An Integer, if set, don't try to automatically
> +          determine what the IP address type is. important for things
> +          like ip_network(1), which could be IPv4, '192.0.2.1/32', or IPv6,
> +          '2001:db8::1/128'.

Same comments

> +def ip_interface(address, version=None):
> +    """Take an IP string/int and return an object of the correct type.
> +
> +    Args:
> +        address: A string or integer, the IP address.  Either IPv4 or
> +          IPv6 addresses may be supplied; integers less than 2**32 will
> +          be considered to be IPv4 by default.
> +        version: An Integer, if set, don't try to automatically
> +          determine what the IP address type is. important for things
> +          like ip_network(1), which could be IPv4, '192.0.2.1/32', or IPv6,
> +          '2001:db8::1/128'.

ditto

> +    Returns:
> +        An IPv4Network or IPv6Network object.

Interface, not Network

> +def v4_int_to_packed(address):
> +    """The binary representation of this address.

Since integers are typically implemented as strings of binary bits, a 
'binary representation' could mean a string of 0s and 1s.

> +
> +    Args:
> +        address: An integer representation of an IPv4 IP address.
> +
> +    Returns:
> +        The binary representation of this address.

The integer address packed as 4 bytes in network (big-endian) order.

> +    Raises:
> +        ValueError: If the integer is too large to be an IPv4 IP
> +          address.

And if the address is too small (negative)? "If the integer is negative 
or ..." ?

> +    """
> +    if address>  _BaseV4._ALL_ONES:

or address < 0?

> +        raise ValueError('Address too large for IPv4')
> +    return struct.pack('!I', address)

It is true that struct will raise struct.error: argument out of range 
for negative addresses, but it will also also do the same for too large 
addresses. So either let it propagate or catch it in both cases. For the 
latter (assuming the max is the max 4 byte int):

try:
     return struct.pack('!I', address)
except struct.error:
     raise ValueError("Address negative or too large for IPv4")

> +
> +def v6_int_to_packed(address):
> +    """The binary representation of this address.

Similar comments, except packed into 16 bytes

> +    Args:
> +        address: An integer representation of an IPv4 IP address.
> +
> +    Returns:
> +        The binary representation of this address.
> +    """
> +    return struct.pack('!QQ', address>>  64, address&  (2**64 - 1))

Why no range check? Here you are letting struct.error propagate.

> +
> +def _find_address_range(addresses):
> +    """Find a sequence of addresses.

An 'address' can in various places be a string, int, bytes, IPv4Address, 
or IPv6Address. For neophyte users, I think you should be clear each 
time you use 'address'. From the code, I conclude it here means the 
latter two.

> +
> +    Args:
> +        addresses: a list of IPv4 or IPv6 addresses.

a list of IPv#Address objects.

> +def _get_prefix_length(number1, number2, bits):
> +    """Get the number of leading bits that are same for two numbers.
> +
> +    Args:
> +        number1: an integer.
> +        number2: another integer.
> +        bits: the maximum number of bits to compare.
> +
> +    Returns:
> +        The number of leading bits that are the same for two numbers.
> +
> +    """
> +    for i in range(bits):
> +        if number1>>  i == number2>>  i:

This non-PEP8 spacing is awful to read. The double space after the 
tighter binding operator is actively deceptive. Please use
         if number1 >> i == number2 >> i:

> +        if (number>>  i) % 2:

ditto

> +def summarize_address_range(first, last):

> +    Args:
> +        first: the first IPv4Address or IPv6Address in the range.
> +        last: the last IPv4Address or IPv6Address in the range.
> +
> +    Returns:
> +        An iterator of the summarized IPv(4|6) network objects.

Very clear as to types.


> +    while first_int<= last_int:

PEP8: while first_int <= last_int:
is *really* much easier to read.

> +        while nbits>= 0:

ditto, etcetera through rest of file.


> +        while mask:
> +            if ip_int&  1 == 1:

Instead of no space and then 2 spaces, use uniform 1 space around &

             if ip_int & 1 == 1:

> +            ip_int>>= 1

To me, augmented assignments need a space before and after even more 
than plain assignments, especially with underscored names.

ip_int >>= 1

I am guessing that Peter dislikes putting ' ' before '<' and '>' and 
perhaps '&', but it makes code harder to read. Putting an extra space 
after is even worse.

This is as far as I read. Some of the style changes could be done with 
global search and selective replace.

---
Terry Jan Reedy