Opened 11 years ago

Closed 7 years ago

#2661 task closed fixed (fixed)

Deprecate IFinishableConsumer so it can be deleted

Reported by: Jean-Paul Calderone Owned by: Thijs Triemstra
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Thijs Triemstra Branch: branches/deprecate-ifinishable-consumer-2661-2
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli, thijs

Description

IFinishableConsumer offers no advantages over IConsumer. IFinishableConsumer.finish is effectively identical to IConsumer.unregisterProducer.

Change History (20)

comment:1 Changed 11 years ago by Peaker

I think that if it is deleted/deprecated, IConsumer should, in addition to unregisterProducer, contain a finish() method.

While unregisterProducer() means "I will not produce any more data for you, but someone else may", finish() would mean "Nobody will ever produce any more data for you, please terminate your consumption and clean up".

This could map to meaningful operations on various consumers. For example, a Process is a consumer that can map "finish" to closeChildFD(0) (loseConnection also closes other process pipes and is not the correct operation when consumption is complete).

Then, to map this extended Consumer to a Protocol via an adapter (as is done in twisted/internet/protocol.py) you would map finish to writeConnectionLost.

A Process would map (as it does today) all consumer methods to childfd(0). Being a FileDescriptor, it would map finish() to a loseWriteConnection, which in the case of a ProcessWriter would convert to a loseConnection.

comment:2 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:3 Changed 7 years ago by Thijs Triemstra

Owner: changed from Jean-Paul Calderone to Ying Li

comment:4 Changed 7 years ago by Ying Li

Author: cyli
Branch: branches/deprecate-ifinishable-consumer-2661

(In [29624]) Branching to 'deprecate-ifinishable-consumer-2661'

comment:5 Changed 7 years ago by Ying Li

Took Peakness's comment and opened a new ticket #4554, which should either be closed as won't-fix or in some way validated.

comment:6 Changed 7 years ago by Ying Li

Er, Peaker, sorry.

comment:7 Changed 7 years ago by Ying Li

(In [29625]) Deprecate IFinishableConsumer, and test that it is indeed deprecated.

refs #2661

comment:8 Changed 7 years ago by Ying Li

Keywords: review easy added
Owner: Ying Li deleted

comment:9 Changed 7 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Ying Li

We should get rid of the one use of IFinishableConsumer in Twisted before putting this deprecation in place. This will both avoid introducing new deprecation warnings emitted from Twisted's internal uses of deprecated APIs and will demonstrate that we really don't need the interface. :)

twisted.protocols.ftp.SenderProtocol is the one implementation we have. I filed #4580 for this.

comment:10 Changed 7 years ago by <automation>

Owner: Ying Li deleted

comment:11 Changed 7 years ago by Thijs Triemstra

(In [31635]) update version, refs #2661

comment:12 Changed 7 years ago by Thijs Triemstra

Author: cylicyli, thijs
Branch: branches/deprecate-ifinishable-consumer-2661branches/deprecate-ifinishable-consumer-2661-2

(In [31636]) Branching to 'deprecate-ifinishable-consumer-2661-2'

comment:13 Changed 7 years ago by Thijs Triemstra

(In [31637]) merge forward, resolve conflicts, and update version nr. refs #2661

comment:14 Changed 7 years ago by Thijs Triemstra

I merged the changes, here are the build results.

comment:15 Changed 7 years ago by Thijs Triemstra

Keywords: review added

comment:16 Changed 7 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Ying Li

Thanks!

  1. The test method docstrings in twisted.internet.test.test_interfaces.py should drop the Ensure prefix. Also I don't think these two things are really testing different cases, but since the tests are already written, whatever.
  2. lookForDeprecationWarning should have a docstring.
  3. The news fragment doesn't follow the style guide. Replace was with is now

comment:17 Changed 7 years ago by Thijs Triemstra

(In [31837]) Address review comments, refs #2661

comment:18 Changed 7 years ago by Thijs Triemstra

Keywords: review added
Owner: Ying Li deleted

comment:19 Changed 7 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to Thijs Triemstra

Looks good to merge.

comment:20 Changed 7 years ago by Thijs Triemstra

Resolution: fixed
Status: newclosed

(In [31908]) Merge deprecate-ifinishable-consumer-2661-2: twisted.internet.interfaces.IFinishableConsumer is now deprecated, use IConsumer instead.

Author: cyli, thijs Reviewer: exarkun, jonathanj Fixes: #2661

Note: See TracTickets for help on using tickets.