Opened 7 years ago

Closed 6 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:

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 6 years ago.
Patch to remove duplicate class of Failure.parents

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by Retenodus

Patch to remove duplicate class of Failure.parents

comment:1 Changed 6 years ago by Retenodus

Owner: set to Retenodus
Status: newassigned

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

comment:2 Changed 6 years ago by Retenodus

Keywords: review added
Owner: Retenodus deleted
Status: assignednew

comment:3 Changed 6 years ago by Dustin J. Mitchell

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 6 years ago by Dustin J. Mitchell

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

comment:5 Changed 6 years ago by Retenodus

Euh, what topfile ?

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

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

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 6 years ago by Jean-Paul Calderone

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 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(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.