Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4928 enhancement closed fixed (fixed)

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

Reported by: exarkun Owned by: lvh
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Branch: branches/allyourbase-4928
(diff, github, buildbot, log)
Author: lvh Launchpad Bug:

Description (last modified by exarkun)

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 ironfroggy 4 years ago.
Patch to remove calls to allYourBase
4928-stop-using-allyourbase-1.patch (1.7 KB) - added by binjured 4 years ago.
Proper use of getmro()

Download all attachments as: .zip

Change History (22)

comment:1 Changed 4 years ago by exarkun

  • Description modified (diff)

comment:2 Changed 4 years ago by exarkun

  • Keywords easy added

Changed 4 years ago by ironfroggy

Patch to remove calls to allYourBase

comment:3 Changed 4 years ago by ironfroggy

  • 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 4 years ago by exarkun

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 4 years ago by djfroofy

  • Owner set to djfroofy
  • Status changed from new to assigned

comment:6 Changed 4 years ago by lvh

  • Owner changed from djfroofy to lvh
  • Status changed from assigned to new

comment:7 Changed 4 years ago by lvh

  • Author set to lvh
  • Branch set to branches/allyourbase-4928

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

comment:8 Changed 4 years ago by lvh

  • Keywords review removed
  • Status changed from new to assigned

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 4 years ago by lvh

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

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

Author: ironfroggy
Reviewer: lvh
Fixes: #4928

comment:10 Changed 4 years ago by lvh

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Reopens: #4928

comment:11 Changed 4 years ago by lvh

  • Owner changed from lvh to ironfroggy
  • Status changed from reopened to new
[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 4 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 4 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 4 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 4 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 4 years ago by binjured

  • Keywords review added

Done. (see attached)

Changed 4 years ago by binjured

Proper use of getmro()

comment:17 Changed 4 years ago by lvh

  • Keywords review removed
  • Owner changed from ironfroggy to lvh
  • Status changed from new to assigned

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

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

comment:18 Changed 4 years ago by lvh

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

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

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

comment:19 Changed 4 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 4 years ago by lvh

Good call. Made #4957.

Note: See TracTickets for help on using tickets.