Opened 3 years ago

Closed 3 years ago

#4957 defect closed fixed (fixed)

Failure.parents has duplicate class

Reported by: lvh Owned by: retenodus
Priority: lowest Milestone:
Component: core Keywords: easy
Cc: Branch:
Author: Launchpad Bug:

Description

In #4928, amaury commented that twisted.python.failure.Failure.parents now contains the initial class twice. This is not a real correctness problem, but it's a bit stupid and inefficient.

Attachments (1)

4957-Failure.parents-has-duplicate-class.patch (418 bytes) - added by retenodus 3 years ago.
Patch to remove duplicate class of Failure.parents

Download all attachments as: .zip

Change History (9)

Changed 3 years ago by retenodus

Patch to remove duplicate class of Failure.parents

comment:1 Changed 3 years ago by retenodus

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

getmro already returns the type, so the line isn't necessary.

comment:2 Changed 3 years ago by retenodus

  • Keywords review added
  • Owner retenodus deleted
  • Status changed from assigned to new

comment:3 Changed 3 years ago by dustin

  • Keywords review removed
  • Owner set to retenodus

This is very straightforward - presumably this error was introduced in the switch from allYourBase to getmro, where the former does not return the given class, and the latter does. I'm tempted to say that this should have tests to verify that the the behavior has changed appropriately, but that seems like overkill here.

The existing tests continue to pass for me. Please merge.

comment:4 Changed 3 years ago by dustin

Er, this also needs a topfiles addition - so do not merge yet!

comment:5 Changed 3 years ago by retenodus

Euh, what topfile ?

I can't merge, the doc says that a core developper will merge it.

comment:6 Changed 3 years ago by exarkun

The topfile can be a .misc for this change I think. I'll add it and apply the patch now. Thanks for your work on this retenodus, and for the review dustin.

comment:7 Changed 3 years ago by exarkun

Ah, also, just to clarify one point. I agree that we don't need a unit test here - because the patch is a pure deletion. Generally it's okay to delete code without adding new tests, as long as some executing tests are executing the code being changed (if no tests had previously been running the if suite including the change, then a new test probably would have been required).

comment:8 Changed 3 years ago by exarkun

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

(In [32306]) Remove duplicate exception from Failure.parents

Author: retenodus
Reviewer: dustin
Fixes: #4957

When twisted/python/failure.py switched from allYourBase to inspect.getmro,
a duplicate of the type of failure.value was introduced into failure.parents.
Remove it.

Note: See TracTickets for help on using tickets.