Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#4928 enhancement closed fixed (fixed)

Replace uses of twisted.python.reflect.allYourBase with inspect.getmro

Reported by: Jean-Paul Calderone Owned by: lvh
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Branch: branches/allyourbase-4928
branch-diff, diff-cov, branch-cov, buildbot
Author: lvh

Description (last modified by Jean-Paul Calderone)

These functions are not exactly the same, but the switch should be pretty easy in most cases. getmro is several times faster than allYourBase because it knows about __mro__. It also takes care of duplication in a diamond inheritance hierarchy, which at least one caller of allYourBase doesn't realize it cares about (Failure doesn't break because of the duplication, but it ends up doing extra work).

Maybe we can eventually deprecate allYourBase and accumulateBases, too.

Attachments (2)

4928-stop-using-allyourbase.patch (1.7 KB) - added by Calvin Spealman 6 years ago.
Patch to remove calls to allYourBase
4928-stop-using-allyourbase-1.patch (1.7 KB) - added by Tom Davis 6 years ago.
Proper use of getmro()

Download all attachments as: .zip

Change History (22)

comment:1 Changed 6 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:2 Changed 6 years ago by Jean-Paul Calderone

Keywords: easy added

Changed 6 years ago by Calvin Spealman

Patch to remove calls to allYourBase

comment:3 Changed 6 years ago by Calvin Spealman

Keywords: review added

The only place the attached patch does not fix is in twisted.persisted, which #3843 calls to deprecate. That is also the only usage of the extra parameter to allYourBase(). Is it required for this ticket to fix the code in a module that is being deprecated?

comment:4 Changed 6 years ago by Jean-Paul Calderone

Thanks ironfroggy! Leaving the deprecated code alone is fine. It's good enough that we'll eventually delete it. :)

(not a review, just answering that question)

comment:5 Changed 6 years ago by Drew Smathers

Owner: set to Drew Smathers
Status: newassigned

comment:6 Changed 6 years ago by lvh

Owner: changed from Drew Smathers to lvh
Status: assignednew

comment:7 Changed 6 years ago by lvh

Author: lvh
Branch: branches/allyourbase-4928

(In [31015]) Branching to 'allyourbase-4928'

comment:8 Changed 6 years ago by lvh

Keywords: review removed
Status: newassigned

Patch looks good, waiting for http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/allyourbase-4928 to not blow up and then I'll merge it. Works on my box at least, and patch looks pretty benign.

Thanks for your contribution!

comment:9 Changed 6 years ago by lvh

Resolution: fixed
Status: assignedclosed

(In [31020]) allYourBase replaced with inspect.getmro

Author: ironfroggy Reviewer: lvh Fixes: #4928

comment:10 Changed 6 years ago by lvh

Resolution: fixed
Status: closedreopened

(In [31023]) Reverting 31020, apparently *does* fail on some buildbots.

Reopens: #4928

comment:11 Changed 6 years ago by lvh

Owner: changed from lvh to Calvin Spealman
Status: reopenednew
[ERROR]
Traceback (most recent call last):
  File "/var/lib/buildbot/bot-bigdogcloud2/lucid64-python2.6-select/Twisted/twisted/trial/test/test_reporter.py", line 1236, in test_subunitWithoutAddExpectedFailureInstalled
    self.removeMethod(reporter.TestProtocolClient, 'addExpectedFailure')
  File "/var/lib/buildbot/bot-bigdogcloud2/lucid64-python2.6-select/Twisted/twisted/trial/test/test_reporter.py", line 1222, in removeMethod
    for base in [klass] + getmro(klass):
exceptions.TypeError: can only concatenate list (not "tuple") to list

So getmro returns a tuple, and you can't concat that to a list.

My suggestion:

for base in [klass] + list(getmro(klass)):

Or possibly:

bases = [klass]
bases.extend(getmro(klass))
for base in bases:

comment:12 Changed 6 years ago by lvh

Okay so scratch that, aumary told me that you should just use getmro(klass) since it already includes the class itself.

comment:13 Changed 6 years ago by lvh

There is of course the caveat that maybe there exists code that appends that output to a list (in fact that seems plausible) so perhaps it should be list(getmro(x))

comment:14 Changed 6 years ago by lvh

We figured out the intermittent failure: tests were being skipped due to dependence on Subunit which was not installed on most slaves and my own machine which is why I was convinced it wasn't broken. Fix is still identical as mentioned in last comment

comment:15 Changed 6 years ago by lvh

#4946 is now blocking on this so it'd be cool if someone could fix it, I have some reviews to do right now so I can't do it for the moment.

comment:16 Changed 6 years ago by Tom Davis

Keywords: review added

Done. (see attached)

Changed 6 years ago by Tom Davis

Proper use of getmro()

comment:17 Changed 6 years ago by lvh

Keywords: review removed
Owner: changed from Calvin Spealman to lvh
Status: newassigned

http://buildbot.twistedmatrix.com/builders/lucid64-py2.6-select/builds/667

Yaay buildbot agrees it is no longer broken. Thanks binjured!

comment:18 Changed 6 years ago by lvh

Resolution: fixed
Status: assignedclosed

(In [31049]) twisted.python.reflect.allYourBase replaced by inspect.getmro

Author: ironfroggy, binjured Reviewer: lvh Fixes: #4928

comment:19 Changed 6 years ago by amaury

One more nit: in failure.py, self.parents now contains the initial class *twice*. For example:

>>> Failure(Exception()).parents
['exceptions.Exception', 'exceptions.BaseException', '__builtin__.object', 'exceptions.Exception']

It's not very important (parents is only used in a statement if err in self.parents:), but it's not optimal.

comment:20 Changed 6 years ago by lvh

Good call. Made #4957.

Note: See TracTickets for help on using tickets.