On Fri, Aug 21, 2009 at 4:41 PM, Nick Coghlan<ncoghlan@gmail.com> wrote:
Peter Moody wrote:
On Thu, Aug 20, 2009 at 10:15 PM, Case Vanhorsen<casevh@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@gmail.com | Brisbane, Australia ---------------------------------------------------------------