[9481] Change DelayedCall repr to include details

[twm]

This turns out to be quite simple, as there is already a __str__ implementation that does the necessary: we can just alias __repr__ to __str__.

Contributor Checklist:

• There is an associated ticket in Trac. #9481 t.i.b.DelayedCall repr should include useful information
• The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
• I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
• I have updated the automated tests.

[codecov]

Codecov Report

Merging #1068 into trunk will increase coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##            trunk    #1068      +/-   ##
==========================================
+ Coverage   91.92%   91.92%   +<.01%     
==========================================
  Files         844      844              
  Lines      150879   150868      -11     
  Branches    13158    13158              
==========================================
- Hits       138694   138685       -9     
+ Misses      10099    10097       -2     
  Partials     2086     2086

[twm]

I moved the epytext syntax fix to #1069

[altendky]

object.__str__() already falls back on __repr__() so I would think it would be tidier to rename the method to __repr__() than to assign the method to both attribute names?

https://docs.python.org/3/reference/datamodel.html#object.__str__

[twm]

@altendky I thought about doing it that way, but then the behavior would be different if __str__ is overridden in a subclass of DelayedCall (which is unfortunately allowed as DelayedCall is public). This way isn't much more code but is more compatible. (This is hypothetical though — the only project I could think of which might need to subclass DelayedCall is txlocal but it doesn't appear to.)

[altendky]

@twm, in what scenario are you saying there would be a difference?

https://repl.it/@altendky/twisted-t9481-pr1068-001

example

class ReprEqStr:
    def __str__(self):
        return "base"
    
    __repr__ = __str__

class ReprEqStrChild(ReprEqStr):
    def __str__(self):
        return "inherited"

print(str(ReprEqStr()))
print(repr(ReprEqStr()))
print(str(ReprEqStrChild()))
print(repr(ReprEqStrChild()))

print()

class Repr:
    def __repr__(self):
        return "base"
    
class ReprChild(Repr):
    def __str__(self):
        return "inherited"

print(str(Repr()))
print(repr(Repr()))
print(str(ReprChild()))
print(repr(ReprChild()))

base
base
inherited
base

base
base
inherited
base

[twm]

Good point... I didn't think that through. If it were implemented like this:

class DelayedCall:
    def __str__(self):
        return '...'
    def __repr__(self):
        return self.__str__()

Then it would be different. I'm torn... does being this fastidious about subclassability even matter here?

Thank you for your attention to detail!

[altendky]

I guess I'm not understanding why we want this functionality (repr getting the str). I wouldn't expect overriding __str__() in a subclass to change the __repr__(). Given the <object ...>-like form It seems more like it was just an initial error that __str__() is what was chosen to be defined (or perhaps the semantics of __str__ and __repr__ were different in 2002 9eaf5b1). I'm probably missing something but I haven't thought of a (not-contrived) scenario where changing the def __str__ to def __repr__ would break existing code. A grep for self.__str__ turns up only one other use (below) which seems like it should just be print(self) or at least print(str(self))), but anyways. So this doesn't seem to be a normal thing for Twisted to do.

twisted/src/twisted/python/usage.py

Line 198 in 5492c16

print(self.__str__())

[twm]

The existing __str__ method generates a __repr__ style string (angle brackets, fully-qualified class name, etc.) so I am inclined to agree that declaring __str__ instead of __repr__ was an error or oversight. The question in my mind is whether the very-strictly-backward-compatible implementation here is worth the 😕 look it will elicit from future readers. Based on this conversation, I think not. If it is, in fact, backwards incompatible in practice we can hope the release process catches it. I'll change the implementation to just define __repr__.

[altendky]

I do generally look forward to better code but I know practically it's appropriate to maintain backwards compatibility. I just don't think this extends to implementation details of _-starting things. But, I won't be offended if you want someone with a bit more experience with Twisted policies to weigh in. :]

Please merge.

[altendky]

(https://twistedmatrix.com is down at the moment, appropriate comment in ticket pending availability)

[twm]

Thank you for the review @altendky! I will merge when the merge forward goes green.