Opened 10 years ago

Closed 10 years ago

#2783 defect closed fixed (fixed)

twisted.internet.task.Clock does not implement IReactorTime

Reported by: David Reid Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve Branch: branches/clock-ireactortime-2783-2
branch-diff, diff-cov, branch-cov, buildbot
Author: dreid

Description

Specifically it has no implements(IReactorTime) declaration and doesn't implement getDelayedCalls.

Change History (16)

comment:1 Changed 10 years ago by David Reid

Keywords: review added
Owner: David Reid deleted
Priority: normalhighest

Addressed in source:branches/clock-ireactortime-2783

comment:2 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to David Reid

Thanks for doing this dreid.

Your branch mostly looks good. It would have been helpful if you explained why Clock should implement IReactorTime in your ticket, but for something this small and obviously useful, I guess it's OK.

Minor quibbles:

  • Pyflakes says that seconds and warnings are imported into task.py without being used. Please remove these imports. (import warnings was added in your branch -- is there a reason for this?)
  • Please add a test that verifies that getDelayedCalls() returns an empty list/set/whatever on a newly-constructed Clock.
  • Please change your test to use assertEqual(set([call, call2]), set(calls)), rather than the len, in, in dance. This makes it clearer that you are testing for equality without regard for ordering.

Once these are done, please land it.

comment:3 Changed 10 years ago by David Reid

Keywords: review added
Owner: David Reid deleted

Ok, i've addressed jml's review points. But I'm going to put this back up for review because it's been a while and opinions might have changed.

comment:4 Changed 10 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to David Reid

The third point of jml's review is still true. Although I may not agree about using set, I'd prefer using sort().

comment:5 Changed 10 years ago by David Reid

Keywords: review added
Owner: David Reid deleted

Seems I accidentally made that change on my trunk working copy which was unbranched from clock-ireactortime-2783. As to using sort instead of set. There is no specification for sorting DelayedCalls so order is undefined and irrelevant.

The version using set is also atleast three lines shorter than a version using sort.

Back for review.

comment:6 Changed 10 years ago by therve

Branch: branches/clock-ireactortime-2783
Keywords: review removed
Owner: set to David Reid

Ahah caught you: python2.3 compatibility, set is not defined.

Otherwise, looks good, so please merge when corrected.

comment:7 Changed 10 years ago by therve

Sorry I forget about one thing: can you add a test to show that a clock provide IReactorTime (and/or is adaptable to). Thanks!

comment:8 Changed 10 years ago by David Reid

(In [22416]) Use Set from the sets module for compatibility, also add test that asserts Clock provides IReactorTime.

Refs #2783

comment:9 Changed 10 years ago by David Reid

Keywords: review added
Owner: David Reid deleted

comment:10 Changed 10 years ago by therve

Keywords: review removed
Owner: set to David Reid

Erm, in the meantime, twisted.compat.set was introduced. Thanks for merging forward and using it :).

comment:11 Changed 10 years ago by David Reid

author: dreid
Branch: branches/clock-ireactortime-2783branches/clock-ireactortime-2783-2

(In [22637]) Branching to 'clock-ireactortime-2783-2'

comment:12 Changed 10 years ago by David Reid

(In [22638]) Merge forward using twisted.python.compat.set Refs #2783

comment:13 Changed 10 years ago by David Reid

Keywords: review added
Owner: David Reid deleted

comment:14 Changed 10 years ago by therve

Keywords: review removed
Owner: set to David Reid

Great, please merge.

comment:15 Changed 10 years ago by David Reid

Resolution: fixed
Status: newclosed

(In [22640]) Merge clock-ireactortime-2783-2: Make task.Clock implement IReactorTime

Add an implements(IReactorTime) definition and implement getDelayedCalls.

Author: dreid Reviewer: therve, jml Fixes #2783

comment:16 Changed 7 years ago by <automation>

Owner: David Reid deleted
Note: See TracTickets for help on using tickets.