Roundup Robot added the comment:
New changeset 038543d34166 by Eli Bendersky in branch 'default':
Switch the AF_* and SOCK_* constants in the socket module to IntEnum.
http://hg.python.org/cpython/rev/038543d34166
--
nosy: +python-dev
resolution: - fixed
stage: patch review -
Ethan Furman added the comment:
Eli Bendersky added the comment:
Another issue is _intenum_converter. Ethan - do you think it belongs in the
enum module as a helper function or something of the sort?
I'm fine with it being a non-public member of enum, although if we had a place
for
Eli Bendersky added the comment:
On Sat, Aug 31, 2013 at 3:18 PM, Ethan Furman rep...@bugs.python.orgwrote:
Ethan Furman added the comment:
Eli Bendersky added the comment:
Another issue is _intenum_converter. Ethan - do you think it belongs in
the enum module as a helper function or
Eli Bendersky added the comment:
[Thanks, Guido]
Attaching patch with SOCK_* constants converted as well. The module globals are
currently walked twice, once to extract AF_* and once SOCK_*. This is not a
real performance problem, but should we fold it into a single loop that
collects two
Guido van Rossum added the comment:
I prefer two prettier loops over one less pretty one in this case. No
opinion about _intenum_converter yet. (IMO refactoring can always be lazy,
i.e. after you have multiple copies of the same code there's time to
consider whether you should unify them and
Guido van Rossum added the comment:
Looks good. Go ahead and convert the rest of the socket constants. Then we
can consider other modules, e.g. select and os.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18720
Eli Bendersky added the comment:
Serhiy, patch 5 addresses your latest review. Thanks. As for _family_converter,
let's see how common this really becomes. Based on one use-case I wouldn't add
it to Enum itself. But if we see it gets repeated in many places, we can.
--
Added file:
Charles-François Natali added the comment:
I've just had a quick look at the patch, but from what I can see,
socket.getaddrinfo() will return a raw integer for the family, and not an
AddressFamily instance. That's unfortunate.
--
___
Python tracker
Eli Bendersky added the comment:
Great catch, Charles-François, thanks. It's actually an interesting example in
favor of the approach - it's very nice to see descriptive family names in the
data returned by getaddrinfo, instead of obtuse numeric values.
The attached patch fixes it in a
Eli Bendersky added the comment:
Looks nice:
for t in socket.getaddrinfo('www.google.com', 'http'):
... t
...
(AddressFamily.AF_INET: 2, 1, 6, '', ('74.125.239.112', 80))
(AddressFamily.AF_INET: 2, 2, 17, '', ('74.125.239.112', 80))
(AddressFamily.AF_INET: 2, 1, 6, '', ('74.125.239.116',
Serhiy Storchaka added the comment:
Besides some nitpicks which I have left on Rietveld the patch LGTM.
I guess the _family_converter idiom will be popular enough. Perhaps we will add
some helper just to the Enum class.
--
___
Python tracker
Charles-François Natali added the comment:
The attached patch fixes it in a similar vein to what was done for the
family accessor: getaddrinfo is overridden in Python and does the necessary
conversion.
Am I the only one thinking that wrapping every C function with a
Python counterpart to
Christian Heimes added the comment:
I wonder how much of an performance issue these wrappers are going to be.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18720
___
Eli Bendersky added the comment:
Charles-François: unfortunately, the alternative seems to be even more tedious
and error prone. Internally, socketmodule.c really uses ints all over the
place. Changing that to use IntEnum objects from Python would be very tedious
and touch a lot more code.
Serhiy Storchaka added the comment:
What about SOCK_STREAM, SOCK_DGRAM, and SOCK_RAW?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18720
___
Antoine Pitrou added the comment:
I'm not really thrilled by the current implementation. The way it is done leads
to duplication among socket constants. Also, if I create a socket from a
numeric value (because a well-known socket family is known yet known by
Python), then socket.family will
Changes by Antoine Pitrou pit...@free.fr:
--
nosy: +neologix
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18720
___
___
Python-bugs-list mailing
Eli Bendersky added the comment:
I'm not really thrilled by the current implementation. The way it is done
leads to duplication among socket constants.
Can you clarify which duplication you mean specifically? In the code review
tool, Serhiy proposed that instead of listing all the constants
Eli Bendersky added the comment:
What about SOCK_STREAM, SOCK_DGRAM, and SOCK_RAW?
This is a proof of concept. As I mentioned in the original message starting
this issue - the SOCK_* constants will get the same treatment as AF_*
constants, once we decide what the right treatment is.
Antoine Pitrou added the comment:
Can you clarify which duplication you mean specifically? In the code
review
tool, Serhiy proposed that instead of listing all the constants
explicitly
in the Python code, we can just do:
AddressFamily = IntEnum('AddressFamily',
{name: value
Serhiy Storchaka added the comment:
I'm surprised that test_repr is not failed. '%i' % socket.AddressFamily.AF_INET
returns 'AddressFamily.AF_INET' instead of '2' as I expected. Is not this a bug
in IntEnum implementation?
--
___
Python tracker
Eli Bendersky added the comment:
Attaching a new patch that uses Serhiy's suggestion to avoid the duplication,
and uses Antoine's suggestion to keep socket.family working even if the family
is an unknown numeric constant.
Note that the latter is challenging to test - I need families the OS
Eli Bendersky added the comment:
On Wed, Aug 14, 2013 at 6:24 AM, Serhiy Storchaka rep...@bugs.python.orgwrote:
Serhiy Storchaka added the comment:
I'm surprised that test_repr is not failed. '%i' %
socket.AddressFamily.AF_INET returns 'AddressFamily.AF_INET' instead of '2'
as I expected.
Ethan Furman added the comment:
Issue18738 created.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18720
___
___
Python-bugs-list mailing list
Christian Heimes added the comment:
The manual list of AF_ prefixes looks ugly. We are going to run into the same
problem for every module. How about an additional constructor that takes a
name, dict and string prefix?
class IntEnum(int, Enum):
@classmethod
def from_moduledict(cls,
Eli Bendersky added the comment:
Christian, this is an interesting idea - but I suggest we review it once we
actually get to convert many modules. For the time being the manual listing
is gone in the recent patch, and for a number of constant families in the same
module (i.e. SOCK_* that will
Eli Bendersky added the comment:
I thing the AddressFamily enum class and the family property should be
documented.
I'm leaving the documentation to the end when we decide on the
implementation strategy. Once that happens I'll post a patch with .rst
changes, etc.
--
Serhiy Storchaka added the comment:
I thing the AddressFamily enum class and the family property should be
documented.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18720
___
Serhiy Storchaka added the comment:
Nitpick: If we have grouped this constants in an enum class perhaps it will be
good if they will be enumerated in the order of its values. Or no?
--
___
Python tracker rep...@bugs.python.org
New submission from Eli Bendersky:
As part of original plan and since issue #18264 has been resolved, it's time to
dust off some old patches I have for the socket.* module. The socket.AF_* and
socket.SOCK_* constants are good candidates for IntEnum conversion.
I'm attaching an initial patch
Changes by Serhiy Storchaka storch...@gmail.com:
--
nosy: +serhiy.storchaka
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18720
___
___
Eli Bendersky added the comment:
Attaching updated patch answering Ethan's review. Also added a tiny sanity test
that makes sure that AF_INET is indeed an enum. A more comprehensive test
probably won't hurt for the final patch.
--
Added file:
32 matches
Mail list logo
