Opened 8 years ago

Last modified 7 years ago

#6904 defect new

TerminalBuffer.reportCursorPosition should return deferred

Reported by: lvh Owned by: Trevor Bramwell
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p, Trevor Bramwell Branch:
Author:

Description

As you can see, the implementation synchronously returns a tuple: https://twistedmatrix.com/trac/browser/tags/releases/twisted-13.2.0/twisted/conch/insults/helper.py#L370

Except the interface says deferred tuple: https://twistedmatrix.com/documents/current/api/twisted.conch.insults.insults.ITerminalTransport.html#reportCursorPosition

The correct thing would be return defer.succeed(that_tuple).

Attachments (2)

return-deferred-from-reportCursorBuffer-6904-1.patch (11.7 KB) - added by Trevor Bramwell 8 years ago.
return-deferred-from-reportCursorPosition-6904-2.patch (12.0 KB) - added by Trevor Bramwell 7 years ago.
Updated patch with NEWS entry.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 8 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 8 years ago by Trevor Bramwell

Cc: Trevor Bramwell added
Owner: set to Trevor Bramwell
Status: newassigned

Changed 8 years ago by Trevor Bramwell

comment:3 Changed 8 years ago by Trevor Bramwell

Keywords: review added
Owner: Trevor Bramwell deleted
Status: assignednew

Patch attached returns a defer.succeed from reportCursorPosition, along with updating tests.

I'm not entirely sure what the deprecation policy is for this sort of change. As the implementation has changed, raising a deprecation warning would not make much sense.

Should this be moved into a separate function, or is there a better way to make this work as a synchronous and asynchronous function at the same time?

Changed 7 years ago by Trevor Bramwell

Updated patch with NEWS entry.

comment:4 Changed 7 years ago by Adi Roiban

Regarding deprecation policy, it depends


Your fix is for implementation... so there is no need to deprecation.


In case documentation needs to be fixed, then you will need to fix the docstring and in case you still want a deferred you need a deprecation warning and a new method..


Do you think that it needs to return a deferred? Does it help with something? Do we have a case where cursor position needs to be returned in an async mode?

Thanks!

comment:5 Changed 7 years ago by Glyph

Keywords: easy review removed
Owner: set to Trevor Bramwell

Hi bramwelt,

Thanks for your contribution.

The purpose of the compatibility policy is to make it so that authors of Twisted programs can upgrade Twisted without worrying that things will break for one revision. This means we lend greater weight to the actual behavior of the implementation than the documentation. Since this suggested implementation would definitely break any existing callers of this method, it's not ideal.

This is an ugly situation, because there's no way to deprecate alternate implementations of the interface.

The formally correct way to do this would be:

  1. make a new interface
  2. correct the documentation of the old interface
  3. deprecate the old interface (with deprecatedModuleAttribute or similar)
  4. make a new method for the new interface
  5. implement the new method everywhere

Note that the new interface and the old interface may happily overlap on all non-deprecated methods.

The real trick is making it so that Twisted's classes can continue implementing the deprecated interface without themselves raising a deprecation warning. You may want to put the old interface in a location where it can be imported under a private name without deprecation by Twisted code, but require typing a forbidden prefixed underscore for other consumers unless they go through the deprecated module attribute path.

Does this make sense?

comment:6 Changed 7 years ago by Trevor Bramwell

Yup, that makes a lot of sense. Thanks for clearing that up for me!

Note: See TracTickets for help on using tickets.