#6570 task closed fixed (fixed)

Port twisted.python.constants to Python 3

Reported by: rwall Owned by: rwall
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/py3-constants-6570
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

I tried to use constants in some new twisted.names code (#5675) but got an attribute error when the module is imported during the Python3 tests.

Traceback (most recent call last):
  File "./admin/run-python3-tests", line 30, in <module>
    unittest.main(module=None, argv=["run-python3-tests", "-v"] + testModules)
  File "/usr/lib64/python3.3/unittest/main.py", line 124, in __init__
    self.parseArgs(argv)
  File "/usr/lib64/python3.3/unittest/main.py", line 168, in parseArgs
    self.createTests()
  File "/usr/lib64/python3.3/unittest/main.py", line 175, in createTests
    self.module)
  File "/usr/lib64/python3.3/unittest/loader.py", line 137, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib64/python3.3/unittest/loader.py", line 137, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib64/python3.3/unittest/loader.py", line 96, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/names/test/test_dns.py", line 21, in <module>
    from twisted.names import dns
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/names/dns.py", line 1821, in <module>
    from twisted.python.constants import ValueConstant, Values
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/constants.py", line 18, in <module>
    _constantOrder = count().next
AttributeError: 'itertools.count' object has no attribute 'next'

Here's a little patch to fix some of the simple stuff...

=== modified file 'twisted/python/constants.py'
--- twisted/python/constants.py	2013-03-20 06:09:10 +0000
+++ twisted/python/constants.py	2013-06-13 22:44:26 +0000
@@ -15,7 +15,8 @@
 from operator import and_, or_, xor
 
 _unspecified = object()
-_constantOrder = count().next
+def _constantOrder(i=count()):
+    return next(i)
 
 
 class _Constant(object):
@@ -180,8 +181,8 @@
         @return: an iterator the elements of which are the L{NamedConstant}
             instances defined in the body of this L{Names} subclass.
         """
-        constants = cls._enumerants.values()
-        constants.sort(key=lambda descriptor: descriptor._index)
+        constants = sorted(cls._enumerants.values(),
+                           key=lambda descriptor: descriptor._index)
         return iter(constants)

But then I run into some further problems which I don't really understand - maybe related to the metaclass

======================================================================
ERROR: test_NULL (twisted.names.test.test_dns.EDNSMessageTestCase)
test_NULL
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/defer.py", line 138, in maybeDeferred
    result = f(*args, **kw)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/utils.py", line 203, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/compat.py", line 286, in reraise
    raise exception.with_traceback(traceback)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/utils.py", line 199, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/names/test/test_dns.py", line 2459, in test_NULL
    msg2 = dns.EDNSMessage.decode(s)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/names/dns.py", line 1912, in decode
    type=MESSAGE_TYPE.lookupByValue(m.answer),
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/constants.py", line 245, in lookupByValue
    for constant in cls.iterconstants():
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/constants.py", line 184, in iterconstants
    constants = sorted(cls._enumerants.values(),
AttributeError: type object 'MESSAGE_TYPE' has no attribute '_enumerants'

======================================================================
ERROR: test_emptyQueryDecode (twisted.names.test.test_dns.EDNSMessageTestCase)
test_emptyQueryDecode
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/defer.py", line 138, in maybeDeferred
    result = f(*args, **kw)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/utils.py", line 203, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/compat.py", line 286, in reraise
    raise exception.with_traceback(traceback)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/utils.py", line 199, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/names/test/test_dns.py", line 2436, in test_emptyQueryDecode
    dns.EDNSMessage.decode(b),
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/names/dns.py", line 1912, in decode
    type=MESSAGE_TYPE.lookupByValue(m.answer),
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/constants.py", line 245, in lookupByValue
    for constant in cls.iterconstants():
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/constants.py", line 184, in iterconstants
    constants = sorted(cls._enumerants.values(),
AttributeError: type object 'MESSAGE_TYPE' has no attribute '_enumerants'

======================================================================
ERROR: test_repr (twisted.names.test.test_dns.EDNSMessageTestCase)
test_repr
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/defer.py", line 138, in maybeDeferred
    result = f(*args, **kw)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/utils.py", line 203, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/compat.py", line 286, in reraise
    raise exception.with_traceback(traceback)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/internet/utils.py", line 199, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/names/test/test_dns.py", line 2285, in test_repr
    repr(dns.EDNSMessage(1)),
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/util.py", line 578, in __str__
    r.append(' %s=%r' % (attr, getattr(self, attr)))
  File "/home/richard/projects/Twisted/branches/edns-message-5675/twisted/python/constants.py", line 43, in __repr__
    return "<%s=%s>" % (self._container.__name__, self.name)
AttributeError: 'NoneType' object has no attribute '__name__'

Attachments (1)

py3-constants-6570-docstringsreflow.diff (2.2 KB) - added by hawkowl 15 months ago.
Patch for 6570 that fixes up some docstrings

Download all attachments as: .zip

Change History (23)

comment:1 Changed 15 months ago by rwall

  • Author set to rwall
  • Branch set to branches/py3-constants-6570

(In [40016]) Branching to 'py3-constants-6570'

comment:2 Changed 15 months ago by rwall

  • Keywords review added

Ready for review in log:branches/py3-constants-6570

comment:3 Changed 15 months ago by exarkun

Seems related to the fact that __nonzero__ is gone - http://docs.python.org/3/reference/datamodel.html#object.__bool

comment:4 Changed 15 months ago by exarkun

Separately, maybe your editor's max column value is set too small. There doesn't seem to be a reason to rewrap most of those docstrings. Is there?

comment:5 Changed 15 months ago by rwall

Thanks exarkun,

You were right about nonzero. I added bool as an alias (as done in t.p.loopback) and all tests now pass.

I also reverted all but one of re-wrapped docstrings.

Added some missing docstrings -- although I'm not entirely sure about my new descriptions.

Raised #6748 because FlagConstants with specific values don't seem to be documented.

Latest Build results:

comment:6 Changed 15 months ago by thijs

This isn't a review but I wanted to point out that the url to the blog post will be outdated and maintenance burden so seems more sensible to omit it.

comment:7 Changed 15 months ago by hawkowl

  • Owner set to hawkowl

Changed 15 months ago by hawkowl

Patch for 6570 that fixes up some docstrings

comment:8 Changed 15 months ago by hawkowl

  • Keywords review removed
  • Owner changed from hawkowl to rwall

Hi rwall, thanks for working on this.

I've looked through and can't find anything wrong with the code. I was a bit iffy about the change from iteritems() to items(), but mithrandi pointed out that unless we're dealing with a million values, the change on py2 is inconsequential at worst, so I think that's fine.

The docstrings on _constantFactory at the very end that you added are reflowed way too early. Attached is a diff to fix them, as well as a early-reflowed docstring in FlagConstant and a too-long comment that twistedchecker picked up, if you think that fixing that up while that file is being changed is fine.

All tests pass, so I think it's good to merge, once the new docstrings are fixed.

comment:9 Changed 15 months ago by itamar

Please read the porting wiki page; there's a bunch of requirements that haven't been met, e.g. no __future__ imports.

comment:10 Changed 15 months ago by hawkowl

  • Owner changed from rwall to hawkowl

...oh huh, I didn't see that - let me rereview this then.

comment:11 Changed 15 months ago by hawkowl

  • Owner changed from hawkowl to rwall

Thanks for pointing that out itamar - my bad, I totally forgot about the porting page, and just looked at passing tests. Anyway...

  1. Line 312 should be isinstance(names, BaseString) or similar
  1. Needs the future imports, as mentioned above.
  1. Docstrings should be updated to use L{bytes} rather L{str}, where appropriate.

Please fix these and then resubmit. It might be better for someone with more extensive py3 experience than I to review it after :)

comment:12 Changed 15 months ago by itamar

Actually, I think str is correct. This isn't supposed to be bytes, this is supposed to be whatever the "native" string format is, since we're mapping them to Python attributes.

comment:13 follow-up: Changed 15 months ago by hawkowl

Ahh, yes, that would be correct, actually. I guess then you don't want to use basestring either, because you're using native strings in both cases. This py3 porting stuff is a lot more in-depth than I originally took it to be.

comment:14 in reply to: ↑ 13 Changed 15 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Ready for another review in log:branches/py3-constants-6570

build results:

Replying to thijs:

This isn't a review but I wanted to point out that the url to the
blog post will be outdated and maintenance burden so seems more
sensible to omit it.

Thijs,

I don't agree with that.

The hack is pretty obscure, so it's useful to have a link to a page
that explains how it works.
If the link breaks it will be a shame, but at least it will give a
clue to someone who's tempted to change that code.

Replying to hawkowl:

Hi rwall, thanks for working on this.

I've looked through and can't find anything wrong with the code. I
was a bit iffy about the change from iteritems() to items(), but
mithrandi pointed out that unless we're dealing with a million
values, the change on py2 is inconsequential at worst, so I think
that's fine.

Yeah, I don't think it will be a problem.

The Python3 porting page also says we should wrap calls to items,
values in list to prevent changes in the API return value. But I
couldn't find any examples of those being returned, so it wasn't
required.

The docstrings on _constantFactory at the very end that you added
are reflowed way too early. Attached is a diff to fix them, as well
as a early-reflowed docstring in FlagConstant and a too-long comment
that twistedchecker picked up, if you think that fixing that up
while that file is being changed is fine.

My wrapping column was set wrong. I've fixed it now and reflowed only
the bits that I've touched in this branch.

Replying to itamar:

Please read the porting wiki page; there's a bunch of requirements that haven't been met, e.g. no __future__ imports.

Done.

Replying to hawkowl:

Ahh, yes, that would be correct, actually. I guess then you don't
want to use basestring either, because you're using native strings
in both cases. This py3 porting stuff is a lot more in-depth than I
originally took it to be.

Discussed with hawkowl on irc and decided that the use of str in
constants is fine.

-RichardW.

comment:15 follow-up: Changed 15 months ago by exarkun

This isn't a review but I wanted to point out that the url to the blog post will be outdated and maintenance burden so seems more sensible to omit it.

Thijs,

I don't agree with that.

The hack is pretty obscure, so it's useful to have a link to a page that explains how it works. If the link breaks it will be a shame, but at least it will give a clue to someone who's tempted to change that code.

It's not "If" it's "When", sadly. Why don't we just include the information somewhere we control so it doesn't disappear? Or link to the Python documentation that explains this? (I'm sure Python *must* have documentation about how metaclasses work!)

comment:16 in reply to: ↑ 15 Changed 15 months ago by rwall

Replying to exarkun:

It's not "If" it's "When", sadly. Why don't we just include the information somewhere we control so it doesn't disappear? Or link to the Python documentation that explains this? (I'm sure Python *must* have documentation about how metaclasses work!)

Ok. The truth is I didn't really understand what the hack was doing, so couldn't explain it.

I've spent some time this afternoon understanding metaclasses and have now modified the hack to be more in keeping with the one in the article.
ie I call type in the class definition to dynamically generate an intermediate baseclass with the correct metaclass.

That required a further change to _ConstantsContainerType.new which was assuming that all its classes would have a _constantType attribute -- which the new dynamic class doesn't.

I've also attempted to explain the hack and linked to relevant official Python documentation.

comment:17 Changed 15 months ago by rwall

This is currently blocking #5446.

comment:18 follow-up: Changed 14 months ago by rwall

Also blocking #6750

In that ticket, glyph wrote a metaclass decorator. r40289

We considered doing the same in this branch, based on six.add_metaclass, but decided that it wasn't necessary.

comment:19 in reply to: ↑ 18 Changed 14 months ago by glyph

Replying to rwall:

Also blocking #6750

In that ticket, glyph wrote a metaclass decorator. r40289

We considered doing the same in this branch, based on six.add_metaclass, but decided that it wasn't necessary.

I'm definitely planning on chucking that code, or at least removing it from that branch; I wrote it to unblock myself because it was less effort than even searching for this ticket, let alone integrating this branch :-).

Looking at the alternate strategy that this branch selected for metaclass declaration though, and the lengthy comment which is used to explain what the hack is doing, I would much rather have the metaclass decorator which can have its own docstring and tests.

comment:20 Changed 14 months ago by glyph

  • Keywords review removed
  • Owner set to rwall

OK, so, I don't think the metaclass thing is ideal - I don't want to have a 5-line comment every time we use a metaclass - but we can easily address this later. Please go ahead and land this.

comment:21 Changed 14 months ago by rwall

  • Status changed from new to assigned

Here's a record of the conversation. Looking back on it, there wasn't a clear
conclusion, but I decided to keep the patch as small as possible because I
wanted it reviewed quickly.

$ less .znc/moddata/log/rwall_freenode_#twisted-dev_20130925.log

  • 13:16:06 <rwall> exarkun: I went looking for authoritative info about the py3 metaclass thing and found this. http://pythonhosted.org/six/#six.add_metaclass
  • 13:16:25 <rwall> Would you rather I copied that into twisted.python.compat?
  • 13:18:30 <rwall> https://bitbucket.org/gutworth/six/src/69df1f152215a67e8942f8616772923308e33b80/six.py?at=default#cl-573
  • 13:19:22 <exarkun> The implementation looks more complicated than constants.py actually needs
  • 13:19:52 <exarkun> I have trouble deciding between these two ideas:
  • 13:20:08 <exarkun> 1) the code should be written in a way that is easy for any python programmer to understand and maintain
  • 13:20:44 <exarkun> 2) metaclasses are not really that complicated and any python programmer should understand how classes are created
  • 13:21:27 * fijal opts for 1)
  • 13:27:09 <exarkun> rwall: okay. if the license is compatible then copying add_metaclass (and its unit tests) and using it sounds fine I suppose.
  • 13:27:10 <exarkun> rwall: when I commented on the ticket, I was only thinking that two or three sentences of explanation should replace the link in the comment, though
  • 13:27:35 <dstufft> six.py is BSD IIRC
  • 13:31:53 <rwall> exarkun: Ok. I'll keep it simple and modify the comment. But I have to admit that I don't fully understand metaclasses yet.
  • 18:44:44 <kenaan> C03rwall C10r40112C14 C07M(branches/py3-constants-6570/twisted/python/constants.py)O: Use a metaclass workaround closer to the original article and the one in six. Attempt to explain the workaround. ...
  • 18:45:02 <rwall> exarkun, itamar: how about that?
  • 18:47:42 <exarkun> I guess /this/ layer of metaclasses is kind of a rathole
  • 18:51:45 <exarkun> rwall: That code is disgusting. But probably only about as disgusting as is necessary.
  • 18:51:59 <rwall> Thanks :)
  • 18:51:59 <exarkun> rwall: I'll just suggest another name than "Py3TmpBase
  • 18:52:01 <exarkun> "
  • 18:52:20 <rwall> Oh that's it?
  • 18:52:27 <rwall> Yeah I can do that.
  • 18:53:03 <rwall> I don't really know what effect that intermedite baseclass has? Is the name seen anywhere?
  • 18:53:12 <exarkun> I think the only other option would be to assemble the class entirely manually (define a bunch of methods, construct the attribute dict, then create _ConstantContainer directly with a call to type - but I don't think we want to go down that path)
  • 18:53:16 <rwall> Also not sure what pydoctor will make of it.
  • 18:53:37 <rwall> That's what they do here: https://bitbucket.org/gutworth/six/src/69df1f152215a67e8942f8616772923308e33b80/six.py?at=default#cl-573
  • 18:53:43 <exarkun> The name can be found out, that's all.
  • 18:53:54 <exarkun> The class persists forever, so "Tmp" seems inappropriate
  • 18:54:01 <exarkun> and it gets used on Python 2, so "Py3" seems inappropriate :)
  • 18:54:02 <rwall> ok
  • 18:54:28 <exarkun> rwall: well yea sort of. They still use the class syntax to create a class, then they rip its insides out and make a new one to replace it.
  • 18:54:57 <exarkun> imagine `def foo(self): ...; def bar(self): ...; FooBar = type('FooBar', (object,), dict(foo=foo, bar=bar))´
  • 18:55:31 <exarkun> (So I guess there are multiple other options, as there usually are. still, they all suck)
  • 18:56:10 <rwall> Cool. That looks like composition.
  • 18:56:10 <exarkun> I think pydoctor will be unable to figure out the base class. Which I don't think matters in this case. No other consequences jump out at me,
  • 18:56:16 <exarkun> rwall: heh
  • 18:56:40 <rwall> ok.
  • 18:58:15 <rwall> Oh one other thing. The "getattr" change won't change the "no ConstantsContainer" subsubclasses rule will it?
  • 19:00:43 <kenaan> C03rwall C10r40113C14 C07M(branches/py3-constants-6570/twisted/python/constants.py)O: use an empty name for the intermediate base class ...
  • 19:07:42 <exarkun> rwall: is there a unit test for that? there should be
  • 19:07:48 <exarkun> rwall: I don't think it will, but it should be tested.
  • 19:08:54 <rwall> Yeah it's ok. The _ConstantsContainer.new method takes care of it.
  • 19:09:47 <rwall> and there are tests for it.

comment:22 Changed 14 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [40299]) Merge py3-constants-6570

Author: rwall
Reviewers: exarkun, hawkowl, glyph
Fixes: #6570

twisted.python.constants is now compatible with Python3

Note: See TracTickets for help on using tickets.