[Josiah] > Error-wise, I agree that it would be better to pass timeout explicitly > with a keyword, but generally users will notice their mistake if they > try to do create_connection(host, port) by ValueError("tuple expected as > first argument, got str instead") Is it better than > TypeError("create_connection takes 1 argument (2 given)") ?
Yes, it is better. Currently, the socket.connect() method takes a tuple, and fails with the following exception if 2 separate parameters are passed TypeError: connect() takes exactly one argument (2 given) Which is fine because the function does take exactly one argument. But we're discussing a function with an optional timeout parameter, so that TypeError wouldn't be raised if I called create_connection("localhost", 80). The patch as it currently is, if I am reading it right, would raise one of the following if a string was passed as the address argument, depending on the length of the string. ValueError: need more than 1 value to unpack # len(address) == 1 ValueError: too many values to unpack # len(address) > 2 since it extracts the host and port like so host, port = address Which succeeds, somewhat surprisingly, if a string is passed that is 2 characters long. I was a little surprised to find that this didn't give rise to an error: host, port = "ab". So with a two character hostname, the second letter would be unpacked as a port number. And the function would then fail with the following exception when it reaches the getaddrinfo ("a", "b", 0, SOCK_STREAM) call. socket.gaierror: (10109, 'getaddrinfo failed') I suggest updating the patch to - Explicitly check that the address passed is a tuple of (string, integer) - To raise an exception explaining the parameter expectation when it is not met - To require that the user explicitly name the timeout parameter Regards, Alan. _______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com