Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Python: Dev

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

 

 

Python dev RSS feed   Index | Next | Previous | View Threaded


tjreedy at udel

May 20, 2012, 10:18 AM

Post #1 of 5 (148 views)
Permalink
Re: [Python-checkins] cpython: Issue #14814: addition of the ipaddress module (stage 1 - code and tests)

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
_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com


solipsis at pitrou

May 20, 2012, 11:29 AM

Post #2 of 5 (148 views)
Permalink
Re: [Python-checkins] cpython: Issue #14814: addition of the ipaddress module (stage 1 - code and tests) [In reply to]

On Sun, 20 May 2012 13:18:29 -0400
Terry Reedy <tjreedy [at] udel> wrote:
> > +
> > + """
> > + if version:
>
> if version is not None: ??
> Do you really want to silently ignore *every* null value, like '' or []?

The goal is probably to have "midnight" mean "auto-detect the address
family" ;-)

cheers

Antoine.


_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com


sandro.tosi at gmail

May 22, 2012, 12:59 PM

Post #3 of 5 (140 views)
Permalink
Re: [Python-checkins] cpython: Issue #14814: addition of the ipaddress module (stage 1 - code and tests) [In reply to]

Thanks Terry for the review! I've attached a patch to issue14814
addressing your points; but..

On Sun, May 20, 2012 at 7:18 PM, Terry Reedy <tjreedy [at] udel> wrote:
>> +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:

I don't see this (and all the other) spacing issue you mentioned. Is
it possible that your mail client had played some "funny" tricks?

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

I don't think I get exactly what you mean here.

Cheers,
--
Sandro Tosi (aka morph, morpheus, matrixhasu)
My website: http://matrixhasu.altervista.org/
Me at Debian: http://wiki.debian.org/SandroTosi
_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com


tjreedy at udel

May 22, 2012, 1:43 PM

Post #4 of 5 (137 views)
Permalink
Re: [Python-checkins] cpython: Issue #14814: addition of the ipaddress module (stage 1 - code and tests) [In reply to]

On 5/22/2012 3:59 PM, Sandro Tosi wrote:
> Thanks Terry for the review! I've attached a patch to issue14814
> addressing your points; but..
>
> On Sun, May 20, 2012 at 7:18 PM, Terry Reedy<tjreedy [at] udel> wrote:
>>> +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:
>
> I don't see this (and all the other) spacing issue you mentioned. Is
> it possible that your mail client had played some "funny" tricks?

Well, *something* between there and here seems to have. I retrieved the
patch in FF browser and that line looks fine. It also looks fine when I
cut and pasted it into a test message from a web mail account to my udel
account, viewed with same mail client. Sorry for the noise. Glad that
you do not need to 'fix' anything of this sort.

>>> + 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.
>
> I don't think I get exactly what you mean here.

This docstring clearly says what the input type is instead of the more
vague 'address'. Also, the output is pretty clearly an iterable of
IPv#Address objects. I meant to contrast this as a good example compared
to some of the previous docstrings.

--
Terry Jan Reedy

_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com


sandro.tosi at gmail

May 23, 2012, 12:56 PM

Post #5 of 5 (131 views)
Permalink
Re: [Python-checkins] cpython: Issue #14814: addition of the ipaddress module (stage 1 - code and tests) [In reply to]

On Tue, May 22, 2012 at 10:43 PM, Terry Reedy <tjreedy [at] udel> wrote:
> On 5/22/2012 3:59 PM, Sandro Tosi wrote:
>> On Sun, May 20, 2012 at 7:18 PM, Terry Reedy<tjreedy [at] udel>  wrote
>>>> +    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.
>>
>>
>> I don't think I get exactly what you mean here.
>
>
> This docstring clearly says what the input type is instead of the more vague
> 'address'. Also, the output is pretty clearly an iterable of IPv#Address
> objects. I meant to contrast this as a good example compared to some of the
> previous docstrings.

Ah now I see, thanks for fixing my understanding ;)

Cheers,
--
Sandro Tosi (aka morph, morpheus, matrixhasu)
My website: http://matrixhasu.altervista.org/
Me at Debian: http://wiki.debian.org/SandroTosi
_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

Python dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.